Already started working on the patch for this, but essentially I want to prevent certain users that have access to "masquerade" from switching to roles with higher privileges than they may currently have. The best way to accomplish this--as I understand now, is to restrict which roles each role is allowed to "masquerade as".

Would like some feedback on this as well.

CommentFileSizeAuthor
#95 permissions__by_role-1171500.patch10.08 KBaurelianzaha
#76 add_masquerade_as-1171500-76.patch8.75 KBfprevos2
#74 add_masquerade_as-1171500-74.patch8.66 KBDavid Radcliffe
#66 add_masquerade_as-1171500-62-63_1.patch8.71 KBBohdan Vasyliuk
#66 interdiff-1171500-62-63.txt1.45 KBBohdan Vasyliuk
#63 add_masquerade_as-1171500-62-63.patch1.66 KBBohdan Vasyliuk
#63 interdiff-1171500-62-63.txt1.66 KBBohdan Vasyliuk
#62 add_masquerade_as-1171500-62.patch7.39 KBmr.york
#62 interdiff-58-62.txt567 bytesmr.york
#58 interdiff_1171500-57-to-58.patch1.71 KBfloydm
#58 1171500-role_defined_access-d7-58.patch7.24 KBfloydm
#57 1171500-role_defined_access-d7-56.patch7.24 KBj_ten_man
#51 1171500-role_defined_access-51.patch6.26 KBjpstrikesback
#51 1171500-role_defined_access-51-tests-only.patch434 bytesjpstrikesback
#47 1171500-role_defined_access-47.patch6.25 KBjpstrikesback
#47 1171500-role_defined_access-47-tests-only.patch429 bytesjpstrikesback
#33 1171500-role_defined_access-33.patch5.92 KBjpstrikesback
#31 role_defined_access.patch5.9 KBjrobison
#25 masquerade-add_permissions_for_each_role-1171500-25.patch17.69 KBmkadin
#25 interdiff.txt834 bytesmkadin
#23 masquerade-add_permissions_for_each_role-1171500-23.patch9.25 KBmkadin
#23 interdiff.txt5.92 KBmkadin
#22 masquerade-add_permissions_for_each_role-1171500-22.patch17.68 KBmkadin
#22 interdiff.txt6.44 KBmkadin
#20 masquerade-add_permissions_for_each_role-1171500-19.patch15.11 KBmkadin
#20 interdiff.txt3.41 KBmkadin
#14 masquerade-add_permissions_for_each_role-1171500-14.patch14.88 KBfuerst
#14 interdiff_1171500-13-to-14.patch1.98 KBfuerst
#13 masquerade-add_permissions_for_each_role-1171500-13.patch14.41 KBfuerst
#9 masquerade-add-masquerade-as-role-permissions-1171500-7.x-1.x-9.patch11.83 KBwjaspers
#4 masquerade-7.x-1.x-dev-1171500-add-masquerade-as-role-permissions.patch23.79 KBwjaspers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wjaspers’s picture

Got it working. Patch to follow soon. Important note: if the patch to follow is implemented, a hook_update will need to be included that removes existing masquerade as use and masquerade as admin permissions, adjusting them to match newly exposed role permissions.

Question to anyone reading this:

Do you use or would you miss the "masquerade admin" roles functionality? If so, why?

One problem I've encountered with the changes here, is that the 'masquerade admin' functionality interrupts role based permissions and I'm stumped as to what do about it. I can a) try to resolve it (will probably take longer), or b) remove it entirely. All feedback welcomed.

EDIT: Finished code changes needed to make masquerade utilize role-based permissions. Caveat: "Masquerade Admin" settings removed. New features: "quick switches" filter out accounts the current user can't access based on permissions, Views field handler added, Context condition/reaction added. Would still like feedback on the above.

bigdave’s picture

We don't regularly use the "masquerade as admin" permission, since our "admin" users are part of an admin role that has all permissions assigned.

I'm interested in this patch.

So are you saying that this creates new module permissions for "masquerade as [role]"?

wjaspers’s picture

Yes I am. I have it working, but it needs a few tests before I can say it's secure.

What's really handy -- in my situation, is that I can assign certain roles to "masquerade as [role]" for customer service purposes. That way I don't need customer service members to spend lots of time tracking down a specific user and adding them to their permitted switches or running the risk of accessing a role they shouldn't have.

I've tried to put some thought into how to handle permissions while masquerading, as well, but I doubt most will need it. (I.E. Limiting what a user that is masquerading can do, even if the role they're masquerading as has a permission to do something.)

wjaspers’s picture

Alright, here's a patch -- which also includes my handlers for CTools the Context module, and Views--just depends on which the maintainers want to process.

I will note, in this patch, I took out the "Test user" fields.

This patch might be best placed in a Masquerade - 2.x branch -- although I have not altered the database schema in any way. Again, its up to the maintainers how they want to handle this.

wjaspers’s picture

Status: Active » Needs review
deviantintegral’s picture

Status: Needs review » Needs work

Thanks for the patch. It looks like it's using Windows line endings, which throws errors when using git apply. Future patches should have Unix line endings to prevent this issue - you might need to change a setting in your text editor to default to Unix line endings.

I think the CTools and Context should be their own issue, and Views integration should be separate as well. In fact, looks like you've started an issue over at #1167744: Add Views, CTools, and Context module support..

Most of my comments are style related that could probably be detected using Coder; I stopped adding comments as there are many lines with identical issues.

+++ b/masquerade.module	Tue Jun 21 17:34:26 2011
@@ -23,15 +22,23 @@
+  // load and build permissions for all roles in our site

Capitalize "Load" and make all comments use sentence case.

+++ b/masquerade.module	Tue Jun 21 17:34:26 2011
@@ -23,15 +22,23 @@
+      $description = t('Grant this permission <em>with caution</em>. Any user signed in is given this role.'); ¶

Fix lines containing trailing spaces.

+++ b/masquerade.module	Tue Jun 21 17:34:26 2011
@@ -23,15 +22,23 @@
+  ¶

Fix lines containing only whitespace.

+++ b/masquerade.module	Tue Jun 21 17:34:26 2011
@@ -232,7 +238,6 @@
-      if ($uid) {

This is removed with no corresponding close bracket?

+++ b/masquerade.module	Tue Jun 21 17:34:26 2011
@@ -757,23 +750,14 @@
+  // ensure the account we're switching to isn't our own and that we have permission to masquerade with these roles

Wrap comments at 80 characters.

+++ b/masquerade.module	Tue Jun 21 17:34:26 2011
@@ -781,6 +765,7 @@
+  // hooray, we made it!

This is redundant.

+++ b/masquerade.module	Tue Jun 21 17:34:26 2011
@@ -836,4 +821,134 @@
+ * Tell ctools where to look for plugins

Should be "Implements hook_ctools_plugin_directory()."

+++ b/masquerade.module	Tue Jun 21 17:34:26 2011
@@ -836,4 +821,134 @@
+ * Adds a masquerading condition to the context module

Same thing here.

+++ b/masquerade.module	Tue Jun 21 17:34:26 2011
@@ -836,4 +821,134 @@
+ * Tells the Context module to look for our custom plugins.

And here.

+++ b/masquerade.module	Tue Jun 21 17:34:26 2011
@@ -836,4 +821,134 @@
+ * Triggers our custom context reactions.
+ * Why isn't this handled by the Context plugin manager??

Here as well. Instead of leaving a question in the comments, we should figure it out and document why.

+++ b/masquerade.module	Tue Jun 21 17:34:26 2011
@@ -836,4 +821,134 @@
+ * Checks if the user has access to masquerade as a specific role.

Add @param phpdoc.

+++ b/masquerade.module	Tue Jun 21 17:34:26 2011
@@ -836,4 +821,134 @@
+ *  Whether or not the current user is allowed to masquerade as someone with this role.

Missing an indentation space.

wjaspers’s picture

@deviantintegral,
What's the best way to split up new features between patches, then? (It sucks developing in a Windows environment)
I have a full patch prepared, but as you mentioned, they should be submitted separately.

Should I submit a patch as if it came after the CTools+Views+Context integrations?

deviantintegral’s picture

In this case, the patches don't have any dependencies on each other. So, write them all against 7.x-1.x without sharing any code. Create a local branch for each patch to make things simpler to track. If you're using all of the patches on a site, you can apply them individually to your specific site's copy of the module. If for some reason a patch depends on another patch, set the issue to "postponed" with a link to the other issue.

git add --patch might be helpful to you, as you can add parts of your changes to the staging area, and do a commit. git stash is also useful when moving uncommitted changes between branches.

wjaspers’s picture

Good to know--thank you for the detailed explanation.

Here's a patch that does NOT contain any the aforementioned Ctools/etc... integration.

wjaspers’s picture

Status: Needs work » Needs review
wjaspers’s picture

Since this hasn't gotten much feedback; would it be appropriate that this patch be useful for a 7.x-2.x branch, wherein the "quickswitch" block is eliminated?

petednz’s picture

Certainly interested in the capability of controlling whether someone with the permission to masquerade should be restricted from masquerading as users with roles x y or z

fuerst’s picture

Refreshed the patch from #9 for 7.x-1.x-dev (2012-Jun-25).
I also added the role based restrictions to the query used to generate the autocomplete list.

fuerst’s picture

Enhanced the patch as following:

* Autocomplete treats a masquerade as Authenticated User permission like expected: Masquerade to all roles allowed
* Use db_select() (DBTNG) instead of db_query()

Note: LIKE is case insensitive in DBTNG so no need for LOWER() anymore.

rosell.dk’s picture

I am certainly also interested in role based permissions. Would have suggested this functionality, but found that the issue was already created. As regards to your question, I have never had any need for the "masquerade admin" permission.

fuerst’s picture

@rosell.dk: Can you try the patch masquerade-add_permissions_for_each_role-1171500-14.patch in #13? When successful please set this issues status to reviewed & tested by the community.

rosell.dk’s picture

Sorry, but I don't have much time on my hands right now. Hope that someone else will review the patch!

wjaspers’s picture

There's a check I introduced in the original patch (and I think it has followed thus far) which looks to see if the user has masquerade as X permissions, which winds up requesting info from the DB several times. I remember seeing something in another issue queue which checks the user's permissions against the user object (and requires fewer trips the DB).
This would be a worthwhile performance benefit. If anyone knows it offhand, it would be great to see this introduced.

mkadin’s picture

This functionality is important to a project I'm working on; we'd love to see it included in the module. I've reviewed fuerst's work and it looks pretty solid. This patch has some minor docs improvements and removed a line or two that didn't need to be there.

The way this patch was set up, you have to have permission to switch to ALL of the roles that the target user has. I think this is the correct approach. The only reservation I have here is the special handling for the 'authenticated user' role.

function masquerade_check_roles($roles = array()) {
  foreach ($roles as $rid => $role) {
    if ($rid == DRUPAL_AUTHENTICATED_RID) {
      continue;
    }
    if (!masquerade_check_role_permission($role)) {
      return FALSE;
    }
  }
  return TRUE;
}

Why should we be skipping over the 'authenticated user' role? Seems to me that this might be confusing behavior, if all of the other roles matter.

mkadin’s picture

And here's my slightly different patch

mkadin’s picture

Status: Needs review » Needs work

There's actually more to the problem discussed in #19. Users that don't have the 'masquerade as authenticated users' permission can still masquerade as any authenticated user.

I could imagine the use case where a site builder wants to have a role called 'masqueradable' and only wants to give certain roles the permission to masquerade as only a class of 'masqueradable' users. This patch would end up allowing them to masquerade as anybody.

mkadin’s picture

Status: Needs work » Needs review
FileSize
6.44 KB
17.68 KB

Here's an updated patch that treats authenticated users as any other role. This means that if you want someone to be able to masquerade as a user with role X, you'll have to give them rights to masquerade as authenticated users and as role X. This is consistent with how the patch treats all the other roles. I think this makes it less confusing and also solves the problem discussed in the first half of #21.

This patch also updates the autocomplete queries to only show users who you can switch to.

Test it out / review it so we can get this in!

mkadin’s picture

I've changed my thinking on the issue of special handling for authenticated users. This patch makes what I was discussing in the 2nd half of my comment from #21 is possible.

If a user has no roles (besides 'authenticated user'), then you have to have the 'masquerade as authenticated user' permission to switch to them.

If a user has other roles besides 'authenticated user', its not necessary to have the 'masquerade as authenticated user' permission as ong as you have permissions to masquerade as all of the other roles.

Though this logic complicates some of the queries and logic, I think its worth what you can get out of it.

GIve it a whirl to test it.

amanaplan’s picture

I agree with mkadin. This does complicate things a bit (both code and UX) but there are times I need the flexibility to give role A the ability to masquerade as users with only role B, but don't want to allow role A to masquerade as users who only have the authenticated user role. I think this makes sense.

Will review/test the patch today or tomorrow.

One thing though, maybe a change in the permission title for "Masquerade as Authenticated User" may help from a UX perspective...something that roughly connotes "masquerade as users who are only members of the Authenticated User role." I'll run it by some tech writers for suggestions that meet clarification and concision needs.

mkadin’s picture

Agreed, this changes the language to 'Masquerade as a user who has no roles besides authenticated user' and also adds the security warning to all masquerade permissions.

amanaplan’s picture

Status: Needs review » Reviewed & tested by the community

This patch has been working like a charm for us in both our test and production environments. Marking as RTBC.

andypost’s picture

Title: Add "masquerade as @role" permissions for each role » Add "masquerade as @role" permissions/settings for each role
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs work

I'd like to commit this feature with proper implementation. So it need re-architecture and re-think settings storage to be more useful so re-titled to point that "permissions" should be stored within module settings.

This needs additional hooks to react on role change, D7 got a lot of them. Needs upgrade path for current settings and probably a tests.

I think current implementation is no-go:
1) it changes a permission model and does not provide a upgrade path
2) it makes permissions page to grow much more
3) adds more queries, some are LEFT JOIN - needs performance testing
4) additional and mostly useless user_load() could be very expensive on sites with big profiles or using OG.
5) code logic and docs makes module much more sophisticated.

+++ b/sites/all/modules/masquerade/masquerade.moduleundefined
@@ -23,15 +23,22 @@ function masquerade_help($path, $arg) {
+  foreach (user_roles() as $rid => $role) {
+    $title = 'masquerade as '. $role;
+    $description = t('Masquerade as a member of the @role role.', array('@role' => $role));
+    if ($rid == DRUPAL_AUTHENTICATED_RID) {
+      $description = t('Masquerade as a user who has no roles besides authenticated user');
+    }
+    $roles[$title] = array(
+      'title' => t($title),
+      'description' => $description,
+      'restrict access' => TRUE,

I really dislike this change. It makes permission page to grow slightly!
I'd prefer to have our 2 permission but provide a matrix of permission internally from admin settings.

+++ b/sites/all/modules/masquerade/masquerade.moduleundefined
@@ -859,3 +885,99 @@ function masquerade_switch_back() {
+ * Checks if the user has access to masquerade as a specific role.
+ * @param string $role

needs blank line between

+++ b/sites/all/modules/masquerade/masquerade.moduleundefined
@@ -859,3 +885,99 @@ function masquerade_switch_back() {
+function masquerade_check_role_permission($role) {
+  $permission = stripos($role, 'masquerade as') ? $role : 'masquerade as ' . $role;

this would break when roles are translated. so needs other way to solve this

+++ b/sites/all/modules/masquerade/masquerade.moduleundefined
@@ -859,3 +885,99 @@ function masquerade_switch_back() {
+ * Checks an array of roles to see if we have access to them.

Really?

+++ b/sites/all/modules/masquerade/masquerade.moduleundefined
@@ -859,3 +885,99 @@ function masquerade_switch_back() {
+function masquerade_check_roles($roles = array()) {
+  foreach ($roles as $rid => $role) {
+    if ($rid != DRUPAL_AUTHENTICATED_RID) {
+      if (!masquerade_check_role_permission($role)) {

Functions needs a better names and doc-blocks.
Role grants a set of permissions. So the comment make no sense at all!!!

+++ b/sites/all/modules/masquerade/masquerade.moduleundefined
@@ -859,3 +885,99 @@ function masquerade_switch_back() {
+ * @param mixed
+ *   The user object to check roles. Also accepts uids and user names.
...
+function masquerade_check_user_roles($user) {
+  if (is_numeric($user)) {
+    $user = user_load($user);
+  }
+  elseif (is_string($user)) {
+    $user = user_load_by_name($user);
+  }
+  elseif (!is_object($user) || !isset($user->roles)) {

So what is the parameter? I think it's possible to unify it

+++ b/sites/all/modules/masquerade/masquerade.moduleundefined
@@ -859,3 +885,99 @@ function masquerade_switch_back() {
+function masquerade_has_any_masquerade_permission() {
+  $return = FALSE;
+  foreach(user_roles() as $rid => $role) {
+    if (masquerade_check_role_permission($role)) {

Useless if we stay with current permissions.

+++ b/sites/all/modules/masquerade/masquerade.moduleundefined
@@ -859,3 +885,99 @@ function masquerade_switch_back() {
+ * Get roles the current user is allowed to masquerade to.
...
+function masquerade_get_not_allowed_roles() {
+  $rids = array();
+  foreach (user_roles() as $rid => $role) {
+    if (!masquerade_check_role_permission($role)) {

Why not simply array_intersect_key() with allow roles?

mkadin’s picture

No problem. Let's clean it up.

Responding to your points:

1) it changes a permission model and does not provide a upgrade path

What would you propose the upgrade path be? If a role has 'masquerade as a user' before, then we give that role permission to masquerade as every role after the upgrade?

2) it makes permissions page to grow much more

I agree it does add rows to the permissions page, and I generally try to avoid that, but I also don't like modules that put their permissions somewhere else. To me it makes sense for all permissions to be in teh same place. But we can move it if you think its necessary.

3) adds more queries, some are LEFT JOIN - needs performance testing

The query changes are in the autocomplete function for the switch user block. This isn't really the critical path for performance, so I don't think that's a big issue. To me, i'd rather have that autocomplete take a few ms longer than have the autocomplete return options for users you can't switch to, which is the purpose of the query's change.

4) additional and mostly useless user_load() could be very expensive on sites with big profiles or using OG.

You're talking about masquerade_check_user_roles($user)? It'd be better to query for the roles directly then instead of loading the whole user object? That makes sense.

5) code logic and docs makes module much more sophisticated.

I'm not sure this can be avoided, but if you have specifics that you think can be done another way, we can definitely clean it up.

From your code comments:

this would break when roles are translated. so needs other way to solve this

I haven't worked much with multilingual sites, perhaps you can suggest a solution.

Really?
Functions needs a better names and doc-blocks.
Role grants a set of permissions. So the comment make no sense at all!!!

Much of these docs / function names were inherited from the earlier folks on this issue, so have no problem changing them, but the docs you're referring to here seem to make sense to me, and I don't know what "Really?" means.

So what is the parameter? I think it's possible to unify it

Whoever authored this function was looking to make it versatile. It's possible to unify it, it will just add more logic throughout the module instead of including it here. I like it as is, but we can change if necesesary. I understand why we want to avoid user_load in this function though.

Why not simply array_intersect_key() with allow roles?

Right now, there is no function that returns all of the allowed masquerade roles...

I'll get started on some of the obvious changes. If you can answer some of these questions, we can keep this thing moving forward.

andypost’s picture

For D8 it's ok to have custom entity-level permissions (#1807776: Support both simple and editorial workflows for translating entities) so better to keep in mind that we have in progress to port the module to D8 #1836516: Port Masquerade to D8 and rewrite + simplify it into a new 2.x series

+++ b/sites/all/modules/masquerade/masquerade.moduleundefined
@@ -859,3 +885,99 @@ function masquerade_switch_back() {
+ * Checks if the user has access to masquerade as a specific role.
...
+function masquerade_check_role_permission($role) {
+  $permission = stripos($role, 'masquerade as') ? $role : 'masquerade as ' . $role;
+  return user_access($permission);

You already introduces this function so simple this one is a access callback for your list of roles.
Remember that user should have overrides for allowed users to masquerade as (and this patch should bring same ability for roles).
That means no reason to introduce new permissions. Simply make this function more useful.

+++ b/sites/all/modules/masquerade/masquerade.moduleundefined
@@ -859,3 +885,99 @@ function masquerade_switch_back() {
+ * Checks an array of roles to see if we have access to them.
...
+function masquerade_check_roles($roles = array()) {
...
+ * Checks all roles of the user we're trying to become.
...
+function masquerade_check_user_roles($user) {

This functions are useful.

But statement that user has access to roles is wrong. This means some custom logic and another argument for own access callback

Kristen Pol’s picture

Hmmm... I need this feature but it doesn't look like anyone is working on it anymore. Anyone planning to soon? :) If not... I might need to figure out this patch and the changes needed per @andypost.

jrobison’s picture

Issue summary: View changes
FileSize
5.9 KB

I saw this issue after working on my own version of this concept.

Please see the attached patch.

Editing to mention this patch is for 7.x-1.0-rc5

deekayen’s picture

I would really like to commit this if it can get rtbc...

jpstrikesback’s picture

Just a re-roll of #31 against 7.x-1.x, with which I will test on simplytest.me :)

jpstrikesback’s picture

#31 works by adding a gate at the end of everything to check if a user has access to switch to a users roles, it works from my testing but there are so many different access checks all over the module that without more testing I can't be sure there isn't unexpected/inconsistent behaviour so I wont be the one to RTBC. Still, I like this approach and someone who knows this module better could likely say if it opens any vulnerabilities.

jpstrikesback’s picture

Status: Needs work » Needs review

The last submitted patch, 13: masquerade-add_permissions_for_each_role-1171500-13.patch, failed testing.

The last submitted patch, 14: interdiff_1171500-13-to-14.patch, failed testing.

The last submitted patch, 14: masquerade-add_permissions_for_each_role-1171500-14.patch, failed testing.

The last submitted patch, 20: masquerade-add_permissions_for_each_role-1171500-19.patch, failed testing.

The last submitted patch, 22: masquerade-add_permissions_for_each_role-1171500-22.patch, failed testing.

The last submitted patch, 23: masquerade-add_permissions_for_each_role-1171500-23.patch, failed testing.

The last submitted patch, 25: masquerade-add_permissions_for_each_role-1171500-25.patch, failed testing.

The last submitted patch, 31: role_defined_access.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 33: 1171500-role_defined_access-33.patch, failed testing.

jpstrikesback’s picture

Little update to test with the new role permissions

jpstrikesback’s picture

Status: Needs work » Needs review

Fire up the bot.

The last submitted patch, 47: 1171500-role_defined_access-47-tests-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 47: 1171500-role_defined_access-47.patch, failed testing.

jpstrikesback’s picture

Status: Needs work » Needs review
FileSize
434 bytes
6.26 KB

One more time.

The last submitted patch, 51: 1171500-role_defined_access-51-tests-only.patch, failed testing.

jpstrikesback’s picture

(hiding files)

deekayen’s picture

I added part of #51 to 8.x-2.x for displaying the multiple permissions options in the main permissions grid, but I didn't get as far as implementing the access controls for the moment masquerading happens or for access control on the block form.

andypost’s picture

Suppose block access is different from URLs that makes switch[back]

+++ b/masquerade.test
@@ -30,6 +30,7 @@ class MasqueradeTestCase extends DrupalWebTestCase {
       'masquerade as user',
       'masquerade as any user',
+      'masquerade as role authenticated user',

this set of permissions looks like overkill, maybe better to simplify this?

For 8.x we can show "block-form" in modal dialog by providing toolbar button

  • Commit 0c543bf on 8.x-2.x by deekayen:
    Issue #1171500 by deekayen@Classic Graphics: start adding support for...
j_ten_man’s picture

Attaching a slightly updated patch for D7. There a few main differences:
- Permissions are based off of rids rather than role names. This avoids translation issues of roles.
- Rather than doing a user_load for each user, do user_load_multiple to hopefully save a little bit of time.
- You can actually switch to the account you select in the block. The previous changes wouldn't allow you to unless you had a higher permssion (masquerade as any user or else permission to masquerade as the given user).
- use format_username rather than just the name. We're loading the user object, so might as well give the user a better experience since we have it.

floydm’s picture

Attached is an update to the patch in #57 reverting the change of permissions to rids rather than role names.

The issue with that change is that if you export permissions via features and deploy to multiple sites, rids for the same role can differ from site-to-site. Thus you can easily end up granting inappropriate permissions.

I get that using the role name is less than ideal but there is precedent for it: in the role_delegation module they use the role name in the permission name.

Status: Needs review » Needs work

The last submitted patch, 58: interdiff_1171500-57-to-58.patch, failed testing.

andypost’s picture

mr.york’s picture

Fixed role detection then authenticated role name translated.

Bohdan Vasyliuk’s picture

Fixed access to masquerade link on user profile view.

andypost’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 63: add_masquerade_as-1171500-62-63.patch, failed testing. View results

Bohdan Vasyliuk’s picture

Bohdan Vasyliuk’s picture

Status: Needs work » Needs review
ron_s’s picture

Status: Needs review » Reviewed & tested by the community

I can't speak to the recent updates for the patch in #63/#66, since we are not displaying the "Masquerade as" link on user pages.

However beyond this, we have been using the patch in #62 for a while, and now the patch in #66 with no noticeable issues. I'm willing to mark as tested, and see if anyone else has any feedback.

izmeez’s picture

Patch in #66 applies to latest 7.x-1.x-dev.

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

izmeez’s picture

The patch in #66 has been now been tried on a couple of installs and while it applies on all those tested in one case when "drush updb" is executed after the patch is applied the following error occurs:

Error: syntax error, unexpected '[' in masquerade.module, line 441

This appears to be related to the following block in the patch:

@@ -419,17 +435,20 @@ function masquerade_user_view($account, $view_mode, $langcode) {
     ->fetchCol();
   $can_masquerade_as_user = in_array($account->uid, $allowed_uids) || user_access('masquerade as any user');
 
-  if (user_access($perm) && empty($account->masquerading) && $user->uid != $account->uid && $can_masquerade_as_user) {
-    $account->content['masquerade'] = array(
-      '#markup' => l(t('Masquerade as !user', array('!user' => $account->name)),
-        'masquerade/switch/' . $account->uid,
-        array('query' => array(
-          'token' => drupal_get_token('masquerade/switch/' . $account->uid)),
+  if (user_access($perm) && empty($account->masquerading) && $user->uid != $account->uid) {
+    if ($can_masquerade_as_user || _masquerade_user_access($account)) {
+
+      $account->content['masquerade'] = [
+        '#markup' => l(t('Masquerade as !user', ['!user' => $account->name]), 'masquerade/switch/' . $account->uid, [
+          'query' => [
+            'token' => drupal_get_token('masquerade/switch/' . $account->uid)
+          ],
           'destination' => $_GET['q'],
-          'attributes' => array('class' => array('masquerade-switch')),
-        )),
-      '#weight' => 10,
-    );
+          'attributes' => ['class' => ['masquerade-switch']],
+        ]),
+        '#weight' => 10,
+      ];
+    }
   }
 }
 

I am not really sure what is wrong with this code or if it has to do with the state of the platform where the error was noted.

ron_s’s picture

The problem is the use of brackets. Drupal 7 supports PHP 5.3, and the use of brackets for arrays is not supported in PHP 5.3.

http://php.net/manual/en/language.types.array.php

As of PHP 5.4 you can also use the short array syntax, which replaces array() with [].

ron_s’s picture

Someone needs to re-roll the patch replacing the brackets with array() for it to be fully valid.

izmeez’s picture

@ron_s Thanks, yes that is it.

David Radcliffe’s picture

I have modified the patch to use the long array syntax. It's working for me, except that I do not see a link to switch back to the original user after masquerading.

andypost’s picture

I still see need to add test coverage we have in d8 branch and take into account points from #2688499: Negate role permissions

fprevos2’s picture

I modified the patch for to handle the "authenticated user" role differently. The permission for "authenticated user" role is only checked when we 'Masquerade as a user who has no roles besides authenticated user'.

In our case, when we give access to "Masquerade as {Role}" we do not want to have to also give access to "Masquerade as authenticated user". For us, it's a security issue to have to grant access to all these users without role to be able to masquerade to a specific role.

ron_s’s picture

@fprevos2, I don't understand what you're trying to do and why it would be a security issue. In fact, I would think what you're suggesting would be a security issue.

Every role in Drupal requires authenticated user by default, and therefore someone should be masquerading as "auth user + {role}", not just "{role}". Please explain in more detail why you consider it a security problem.

Also it's not appropriate to change the foundation of a patch that we've been trying to get approved for 5+ years with a different fundamental design and is already marked as RTBC. As well, almost every module I work with assumes that if someone is granted permission for a given role, it applies to the auth user role too.

izmeez’s picture

I support the comments in #77. I cannot see any reason why masquerade would be available to users with only the authenticated role. This is the type of module that requires elevated privileges for support or administration.

solideogloria’s picture

Agreed. Even if your own organization uses the module this way somehow, there's no need to break masquerade for everyone else just to get your specific use-case in.

fprevos2’s picture

My intention where to provide a work around for people with the same issue them me. I didn't expect my patch to be applied or select as the proper patch.

But let consider the following scenario to explain why I changed it. Let say that my site as the following 3 users. User A has no role other than authenticated user. User B is also a Content editor. User C as a masquerade role. And let say I want user C (masquerade role) to masquerade as user B (Content editor role).

With the default patch, I need to give user C's role access to both "masquerade as authenticated user" and "masquerade as Content editor". This as the side effect of letting user C also masquerade as user A.

With my patch, I only need to give access to "masquerade as Content editor" and then user C cannot masquerade as user A.

I personally don't see how my patch is less secure. Could any one of you explain why it would be less secure?

Thanks

ron_s’s picture

@fprevos2, I don't think there is a full understanding of how Drupal's permissions structure works. It is additive to auth user.

All users are authenticated users by default, and all users have authenticated user permissions. When you add a new role into the system, it is not a replacement for the authenticated user role, but rather in addition to it.

If you are granting access to "masquerade as Content editor", you *must* give that user "masquerade as authenticated user", otherwise you are not truly masquerading as what a Content editor sees. The permissions for a Content editor in your system are not only the Content editor permissions, but "authenticated user + Content editor".

It absolutely needs to be both of these together, or else someone masquerading as the Content editor could potentially be missing any feature that is solely shown to authenticated users.

fprevos2’s picture

I understand how permissions/roles works in Drupal I've been using it for a while.I think you don't fully understand how masquerade actually work. I agree that with the current patch you need to give both permissions for it to work. But once you masquerade as that person you have their permissions/roles, it does not matter what your own account has as permission at that point. While you are masquerading as that user B, you could remove all masquerading access to your own original account and you would still see exactly what that user B see because at that point it does not check the masquerading access anymore.

So with my patch, when you are masquerading as a "Content editor" the user your are masquerading as still has both roles "Content editor" and an "authenticated user". The point of my patch is just to prevent you from being able to masquerade to all the other users that are just "authenticated user" and have not other roles like my example "User A".

fprevos2’s picture

@ron_s instead of arguing over what we think the behaviour is/should be, would you mind just testing my example/use case with the patch from #74 and then re-testing the same scenario with my patch from #76?

Thanks

solideogloria’s picture

@fprevos2 I agree with your first paragraph, but you are definitely wrong on the 2nd.

Every user with masquerade access should be able to masquerade as every user that is just an "authenticated user". Higher-privileged users (such as Content Editor or Administrator users) might require a user to have additional access to masquerade as them, but if user A has access to masquerade as a Content Editor, they should still be able to masquerade as everyone else, too. That's how it is supposed to be.

I think maybe what you desire is a subtractive access model or some other model instead of an additive one.

And no, we don't need to test your patch, because the behavior is not at all what anyone else is looking for. Go create your own custom module or a separate issue if you think it is useful. Your patch doesn't work for anyone who uses Masquerade the way it was meant to be used, but instead changes the core functionality and definition of what the module is and how it works.

Berdir’s picture

FWIW, #2688499: Negate role permissions has been committed without special-casing the authenticated role.

I don't think there is a security issue either way, as there's no way that an authenticated user would have more permissions than any other role.

That said, I agree that the patch from @fprevos2 is an interesting improvement that allows for some use cases that are otherwise not possible *and does not prevent any existing use case*.

Every user with masquerade access should be able to masquerade as every user that is just an "authenticated user". Higher-privileged users (such as Content Editor or Administrator users) might require a user to have additional access to masquerade as them, but if user A has access to masquerade as a Content Editor, they should still be able to masquerade as everyone else, too. That's how it is supposed to be.

There's nothing that states that this is how it "has" to be.

Again, every use case is still possible. The way the patch works now is that if you grant someone the "masquerade as authenticated" permission, then they can *only* authenticate as those. And if you grant someone that permission *and* "masquerade as editor", then they can masquerade as users with only authenticated and users with the editor (and implicit authenticated role).

However, if you have a use case where you want to allow users of role A to *only* masquerade as editors but not as users without any role, then don't grant them "masquerade as authenticated". There are valid use cases for that, for example not permission but privacy related. You might not want users from role A to see private fields of all authenticated users (without extra roles). The new patch allows this, the previous patch did not as it forced you to grant "masquerade as authenticated".

fprevos2’s picture

@solideogloria Based on your explanation subtractive vs additive model, I understand why the "current" behaviour might be wanted by most people and if this is the case I'm fine with that being committed instead of my patch.

Sorry if you feel like I'm "hijacking" this issue but when I start looking into the issue I when through a bunch of the comments and saw someone mention a similar behaviour (comment #24). Then I decided it was the approach that would also make sense for me and created the patch based on the current patch from #74. I though it was relevant and could be useful to someone else so I decided to post it here.

I don't think it would make sense to create a new module for what I want. If the patch from #74 ever get committed, which I hope it does, I'll just rewrite my patch to apply it on top of the new version.

And I know that no one need to test my patch I was just asking @ron_s to test it because I though he would understand what it does better if he actually tried it out.

I'm sorry that you all seem to feel offended and that you think I was trying to break masquerade (when my patch actually does not break anything). I was just trying to help other and trying to contribute back in a community that is supposedly open.

ron_s’s picture

@fprevos2, I understand what you are attempting to accomplish. What you seek is, for example, the following: a user with masquerade authority starts to type a username in the masquerade admin dropdown select. The user only has the ability to masquerade as Content admins, so only the name of Content admins show up in the list. Authenticated users without the Content admin role will not be displayed. This restricts the masquerader to only masquerade as a user if the given user has the Content admin role.

I don't view this as a security issue, because as @solideogloria said, every user with masquerade access should be able to masquerade as every user that is just an authenticated user. This is essentially the baseline functionality of the module if a site has no additional rules.

I've looked at your code change. You're modifying the default functionality of _masquerade_user_access, which changes how this works for everyone. (Note: you should create an interdiff if you want everyone to more easily understand what you've done: https://www.drupal.org/documentation/git/interdiff) This should be an optional choice, not mandatory, and certainly not the default setting.

While I can understand this may be an important use case for your needs, this really should be its own patch as a separate feature request.

fprevos2’s picture

@Berdir Yes, I guess my goal was to be able to protect the information (name/email/...) of authenticated users without other roles. So it could be considered a privacy issue more than a security itself. By restricting access to only "Content editor", we block masquerade access that would have allow you to see the private information on the user pages that all the users without any other roles than "authenticate users".

fprevos2’s picture

@ron_s In my case

I don't view this as a security issue, because as @solideogloria said, every user with masquerade access should be able to masquerade as every user that is just an authenticated user. This is essentially the baseline functionality of the module if a site has no additional rules.

is not what we want. I could be wrong but I think it's currently not the case without this patch either. I have not test it out but when I looked at the code it seems like you can only can only masquerade to specific users (defined in the table masquerade_users) unless you have the permission "Masquerade as any user" which is different then "Masquerade as user".

And thanks for the link about interdiff, I'll look into it.

solideogloria’s picture

@fprevos2 Since your profile shows you work for an educational institution, you should consider using Organic Groups for access. Have a group for each user type, and users can be in more than one group. Then you can more finely control who can see what, including masquerade access.

fprevos2’s picture

@all thanks for the feedback.

@solideogloria funny thing is we are already using OG to control content access and it is one of the reason why we have masquerade. But I didnt know I could use it to control masquerade access. Thanks.

I put my patch as hidden to no confuse the maintainer of the module with it. Not sure if there is something else I need to do for it to be ignored.

Hopefully this issue is committed soon.

izmeez’s picture

The discussion on the new patch in #76 by @fprevos2 has been enlightening and the comments in #85 provide a clear perspective. I am inclined to change my view on the merits of this approach and as said in #85

the patch from @fprevos2 is an interesting improvement that allows for some use cases that are otherwise not possible *and does not prevent any existing use case*.

The new patch is more about privacy than security. In light of this we will do more testing of the patch. Thank you.

klausi’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/masquerade.module
    @@ -44,6 +44,17 @@ function masquerade_permission() {
    +    $perms[$perm_string] = array('title' => $perm_title, 'description' => $perm_title);
    

    Switching users is a security critical operation and must only be done by admins. Therefore all masquerading permissions must be marked as restrict access => TRUE.

  2. +++ b/masquerade.module
    @@ -604,6 +623,8 @@ function masquerade_block_1() {
    +        if(!_masquerade_user_access($account)) continue;
    

    please follow drupal coding standards and always use curly brackets for if() bodies. This is security critical code and reminds me of the Apple Goto Fail security vulnerability where developers forgot braces https://embeddedgurus.com/barr-code/2014/03/apples-gotofail-ssl-security...

  3. +++ b/masquerade.module
    @@ -915,3 +956,61 @@ function masquerade_switch_back() {
    +  if (user_access('masquerade as admin')) return TRUE;
    

    the main permission is "masquerade as any user", right?

klausi’s picture

Sorry, forgot to add: Otherwise I think the approach makes sense and if we fix up the minor issues this should be ready!

  1. +++ b/masquerade.module
    @@ -723,11 +746,19 @@ function masquerade_autocomplete($string) {
    +  foreach ($result as $record) {
    

    no need to iterate, use ->fetchCol() on $result.

  2. +++ b/masquerade.module
    @@ -761,11 +792,19 @@ function masquerade_autocomplete_multiple($string, $add_anonymous = TRUE) {
    +    foreach ($result as $record) {
    

    same here

aurelianzaha’s picture

aurelianzaha’s picture

Status: Needs work » Needs review
klausi’s picture

Thanks Aurelian, this looks pretty good now. You are using short array syntax, which could be a problem for sites still on PHP 5.3. I think at this point we don't care much about that anymore, even the testbot tests with PHP 5.5 - which is unsupported as well.

+++ b/masquerade.module
@@ -915,3 +963,59 @@ function masquerade_switch_back() {
+ * @param type $account

type should be object

So not setting this to RTBC for now, waiting for others to confirm if anyone is still on PHP 5.3, otherwise we could go ahead here.

izmeez’s picture

On the question of keeping the old array() syntax and support for php 5.3 there is a D7 core discussion on this in #3155190: [meta] Should Drupal 7 drop support for older PHP versions?. While support for anything before 5.6 is weak there still appears to be a willingness to continue with old array() syntax.

mvc’s picture