Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

You need the patch from #1555862: DrupalWebTestCase::drupalGetToken() does not add hash salt to make the test work.

wjaspers’s picture

Are you proposing this goes into core or just that masquerade gets rolled ahead to D8's new module/plugin architecture?
Either way, there are a few lingering feature requests in the queue that would be worth rolling up.

andypost’s picture

Yay! Thanx a lot for #1555862

The only thing that scares me - masquerade as anonymous which could be dropped for core

For D8 there's a strange situation with session handling and we need more reviews for #1813696: Set/unset Session flag when masquerade status changes.
Next one is clean-up #1835954: Cleanup code and allow pass tests that broken because of token generation

sun’s picture

Can you open up a 8.x-1.x branch and commit #0?

Yes, there are definitely quite a range of things that can be vastly simplified first, potentially dropped later.

Also, bug reports like #1813696: Set/unset Session flag when masquerade status changes. demand for test coverage. As soon as the core patch is in, the testbot should be enabled for the 8.x-1.x branch.

sun’s picture

FWIW, I could also commit it myself? *wink* ;)

andypost’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Needs review » Needs work

I've commited your initial patch and created a new 8.x-1.x-dev release

deekayen’s picture

Re #3, if you've followed the masquerade history for a couple years, the masquerade as anonymous feature has been introduced, then pulled, then re-introduced, then pulled, in a cycle. It's never really been a good idea. My proposal is if you want to masquerade as anonymous, then logout. I'd prefer to make that policy stick and yank the functionality back out of masquerade.

sun’s picture

+1 to that. I don't really see the point in masquerading as anonymous either.

Also, sorry for getting silent here. Work on this initial porting patch allowed me to study what exact functionality is currently supported by Masquerade module, and I've to admit, that's far more than I remotely expected.

Therefore, I was mainly busy with "thinking" ;) and trying to make sense of the questions:

If we'd move this into core, which features would be the 80% use-case? And how would we ideally implement it as a built-in core feature? And how could a masquerade_advanced contrib module bring back the other 20% that we removed, without re-inventing the functionality from scratch?

Additionally, there's some functionality in Masquerade that I didn't expect to see, and for which I rather expected to see Devel module's rather simplistic behavior; namely, the (limited) list of default "switch users".

andypost’s picture

The "limited list of users" we have as block on per-user basis. Also there's a request to re-implement the logic to allow masquerade on specific roles only. #1171500: Add "masquerade as @role" permissions/settings for each role
I'd like to make integration for admin_menu the same as devel does because I always use both modules in my dayly job

sun’s picture

Yeah, to clarify some further from my side:

I'd ultimately like to replace admin_menu's built-in support for Devel (completely) with Masquerade. That is, because masquerading is what >80% of all developers/users actually want and need; switching user sessions is not exactly the desired functionality.

Meanwhile however, I rather strongly believe that the current Masquerade module is too complex and provides too many advanced features, options, and settings. Due to that, I would always think twice, before considering to install the module on a site.

I wonder whether you'd be open to do some larger changes? I'd imagine these steps:

  1. Research and identify the most common and basic >80% functionality.
  2. Split the module into masquerade.module and masquerade_advanced.module.
  3. Only include the basic functionality in the main Masquerade module. (I expect that to be a fairly simple & very small module.)
  4. Everything else is shifted into the advanced module (or some too advanced features perhaps even dropped entirely).
  5. Keep the basic/main module really really basic. Resist the feature-creep ;)

What do you think?

Not having thought about it too much yet, but here are the components that I'd foresee in the basics:

  1. A single user permission to "Masquerade as another user".

  2. A (tl;dr) "widget" that contains a link "Masquerade". Supposed to be attached to the user account name in (whichever) toolbar.

    masq-toolbar-link.png

  3. This widget essentially allows for a few, discrete interactions:

    1. Direct selection of a target user, through a directly exposed dropdown list of recently registered / recently masqueraded-to users.
    2. Autocomplete input selection of a target user, through a directly exposed autocomplete input widget (in addition to the dropdown list).
    3. Clicking "Masquerade" itself navigates to /admin/people, which in turn is the regular [admin_]view, but the Operations for each listed user account additionally include a new "Masquerade" operation.
  4. In case the masqueraded user also has access to (whichever) toolbar, then the user account name receives some special styling to indicate so, and also allows to unmasquerade.

    masq-toolbar-active.png

  5. Otherwise (or any in case), the Account links (secondary links) menu is always enhanced to additionally contain a "Unmasquerade" link.

    masq-account-links-active.png

And that's... more or less all. No block. No user account settings. No role-specific advanced stuff, or whatnot.

Just the 80%, and just simply that.

andypost’s picture

whether you'd be open to do some larger changes?

Sure! The UI looks really awesome!

...current Masquerade module is too complex and provides too many advanced features, options, and settings...
Split the module into masquerade.module and masquerade_advanced.module.

Agreed here, but probably better to move this "features" into plugins.

Research and identify the most common and basic >80% functionality.

Suppose "masquerade as" fixed user list.

Everything else is shifted into the advanced module

Or probably the "d8-plugins" enabled in settings form.

Keep the basic/main module really really basic

block & functions to [un]switch users.
Autocomplete also needs review because core has one & #1171500: Add "masquerade as @role" permissions/settings for each role suggests to filter results by allowed users only

sun’s picture

On plugins:

Well, hm. My baseline thinking is to significantly simplify the main masquerade.module. Yes, plugins are all-optional and stuff, but nevertheless, they directly imply more code that is loaded, more stuff that is exposed in the UI, more code to maintain, more complexity, and many more things.

Unfortunately, we did not meet the deadline for the D8 feature freeze here — but the essential point of this effort would be to

  1. Make the main/basic Masquerade module the standard go-to solution for literally everyone.
  2. Simplify it on all fronts.
  3. Optimize the code and the user interface and experience. Aim for a deep integration with toolbar implementations.
  4. Prepare the entire thing for D9 core inclusion, and retain its quality and simplicity until that happens.
  5. Potentially even ask Devel maintainers to drop its switch-user functionality in favor of Masquerade.

Everything that isn't part of the optimized 80%/main use-case is moved away into another (sub-?)module. The vast majority of users does not need that stuff and should not need it in the first place.

Everyone who installs Masquerade module should be able to say:

  • This is helpful. Even though I don't know whether I'll actually need it.
  • This is secure. 100% test coverage for the very limited functionality it provides.
  • This isn't bloat. Super small, dead simple, focused on the >80% task.
  • This is friendly. Available when I need it, close to "zero-conf", and has absolutely no other UI implications.

The last bullet might actually be the most important, as it summarizes what I tried to get to earlier:

If you look at the admin_menu + devel integration, then that's the exact experience I'm aiming for — There's only a single "switch users" permission, and that's it.

Thus, you just enable the modules, grant that permission, and waka-boom, you're fully set. There are no blocks to configure, no plugins in the block library that get in your way, no obscure fields or settings on user accounts, no giant list of complex permissions, nor anything else. It's 100% focused, immediately available when you need it, and very intuitive. The only problem with Devel is that Devel has no business on a production site. (Next to switching user sessions instead of masquerading.)

And that's where this revised Masquerade module comes in: Make it bad-ass simple. And rock solid. And nothing else.

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
30.05 KB

Attached patch demonstrates what I'm after. (It's only the starting point though.)

53% of the code + complexity just simply goes away.

This impacts the most trivial and basic things only. There's a second ~25% chunk of code that can be vastly simplified.

Essentially, the goal is:

  1. Remove everything, except of the ~20% that make up the main 80% functionality.
  2. From there, optimize the user experience by deeply integrating into the user interface and toolbar solutions.
  3. And with that, aim for no less than 100% test coverage. (Currently 0%.)

That said, honestly, I do not care at all for the 70-80% of add-on code and functionality that would be removed during this course. I only care for the most common + main 20% functionality, as outlined in #12. Therefore, someone else would have to pick up the responsibility for that removed code, and re-implement (and maintain and test) it in a separate sub-module or separate d.o project. (I'd prefer the latter.)

Or in other words: If you need more options, do it yourself. We'll make sure to provide a sophisticated API. The contributed masquerade_advanced.module might serve your needs, but I/we don't really care.

sun’s picture

FileSize
50.41 KB

Alright. I discussed this some more with @andypost in IRC last night.

Attached patch does not make any further substantial progress towards #10 + #12, but instead of deleting all the existing code, the advanced functionality is moved into a new masquerade_advanced.module.

Note that I did not test the code (except of installing), because testing is obviously not possible without the actual new user interface of #10 + the deep integration into toolbar modules. Additionally, there's quite some code that needs to be updated for latest changes in core/HEAD. In other words, the main masquerade.module does not really do anything with this patch.

Nevertheless, I'd like to move forward with this patch and commit it as an initial milestone. Afterwards, I'd like to continue to work on the new UI/integration.

Speaking of, here's a more concrete plan I'd propose:

  1. Create a new 8.x-2.x branch. Let's commit this patch to 8.x-2.x only.

    This is a serious rewrite of Masquerade module, and a new major release will be appropriate on all fronts — it clarifies that there are API changes, that there are UI changes, and sends out the right message to everyone.

  2. After completing work for 8.x-2.x, we can investigate to backport the 2.x series to D7.

    I think I'd be interested in that. It would also help me to eliminate (or at least deprecate) the Devel integration in admin_menu a bit sooner.

  3. I'd love to get commit access and maintain the 2.x branches. :-)
andypost’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev
Status: Needs review » Needs work

@sun Thanx a lot for your patient answers in IRC tonight!

I decided to move forward here with 8.x-2.x branch http://drupal.org/node/1926048
Also added @sun as maintainer!!!

The roadmap:
1) Split module on masquerade and masquerade_advanced
2) Masquerade module just simple api + toolbars integration
3) advanced module should add features like block, switch links, roles (#1171500: Add "masquerade as @role" permissions/settings for each role )

Questionable moments covered:
1) permissions: latest patch introduces hook_masquerade_access() which should solve access issues
2) api functions masquerade_switch_user() masquerade_switch_back() seems should not be involved in access checks
3) test coverage

sun’s picture

Status: Needs work » Needs review
FileSize
18.31 KB

Thanks! :)

I've committed and pushed #14 to 8.x-2.x.

Attached patch serial:

  1. Fixed missing module dependency declarations.
  2. Restores the Masquerade block (autocomplete form).
  3. Introduces the "Unmasquerade" link in the Account links menu.

I.e., with this patch, the basic functionality appears and is usable again. Note that I consider the block as a temporary measure only.

I also created #1926074: Remove {masquerade} table and rely on session flag only

sun’s picture

FileSize
23.18 KB

Additionally fixed the tests by working around the drupalGetToken() core bug for now.

andypost’s picture

+++ b/lib/Drupal/masquerade/Plugin/block/block/MasqueradeBlock.phpundefined
@@ -0,0 +1,69 @@
+   * Overrides \Drupal\block\BlockBase::settings().
+   */
+  public function DISABLEDsettings() {
+    return array(
+      'block_count' => 10,

I think that's useless

+++ b/masquerade.moduleundefined
@@ -80,37 +80,34 @@ function masquerade_menu() {
+    // Invoke masquerade_translated_menu_link_alter() to append token.
+    'options' => array('alter' => TRUE),
...
+    // Invoke masquerade_translated_menu_link_alter() to append token.
+    'options' => array('alter' => TRUE),
...
-function masquerade_menu_alter(&$items) {
-  $items['masquerade/switch/%']['options']['alter'] = TRUE;
-  $items['masquerade/unswitch']['options']['alter'] = TRUE;

This does not works for D7, so not sure D8 supports this

+++ b/masquerade.moduleundefined
@@ -152,7 +149,7 @@ function masquerade_user_operations_masquerade(array $accounts) {
- *   Either 'switch', 'unswitch', 'user', or 'autocomplete'.
+ *   Either 'switch', 'unmasquerade', 'user', or 'autocomplete'.

@@ -162,11 +159,8 @@ function masquerade_user_operations_masquerade(array $accounts) {
+    case 'unmasquerade':
+      return isset($_SESSION['masquerading']);
 
     case 'switch':

I think if we going to rename this so this name should be similar
switch|unswitch => masquerade|unmasquerade

andypost’s picture

FileSize
1.82 KB

Commited conversion of HTTP_REFERER

sun’s picture

FileSize
12.58 KB

Since #17 re-establishes the basic module functionality, I've committed that. :)

Also enabled the testbot for the 8.x-2.x branch. Yay!

Attached patch:

  1. Changes /masquerade/switch/% into /user/%user/masquerade ([inline] local task/operation).
  2. Replaces the hook_user_operations() implementation with a form alter to add a Masquerade operation link for each account on /admin/people.
sun’s picture

FileSize
12.58 KB

Sorry, I just noticed that there's an additional flag for d.o projects that toggles whether the testbot is enabled.

Same as #20.

sun’s picture

Status: Needs review » Active

Committed and pushed to 8.x-2.x. Yay! :)

andypost’s picture

Status: Active » Needs review

#21 is great idea. Tab on user page and removal of operations in favor of operations link (should be added to reeadme)
Also #1555862: DrupalWebTestCase::drupalGetToken() does not add hash salt still broken

sun’s picture

FileSize
9.26 KB

Revamped masquerade access and UI validation.

sun’s picture

FileSize
5.14 KB

Committed #24.

Now: Added masquerade_user_is_masquerading() helper function.

sun’s picture

Status: Needs review » Active

Committed #25.

Alrighty! With that, we have a pretty awesome clean slate :)

masquerade.module measures 13.8 KB now, which is ~46% of its original size. I'll look into #1926074: Remove {masquerade} table and rely on session flag only now, which will shave off another good chunk of bytes.

andypost’s picture

Status: Active » Needs review
FileSize
2.72 KB

A bit of cleanup needed

Status: Needs review » Needs work

The last submitted patch, masquerade_cleanup.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
3.74 KB

Somehow global $user object could be not User entity

andypost’s picture

Status: Needs review » Active

Commited

sun’s picture

Status: Active » Needs review
FileSize
10.57 KB

A dozen of further clean-ups.

Some issues that occurred to me:

  1. I don't really understand why one is not allowed to masquerade as a user with lesser permissions when maintenance mode is enabled. I'll have to check the git history to find out which issue introduced that validation, and why (or for which use-case).
  2. The hook_user_view() implementation + standalone link on the user account page sucks big time. I'd like to remove that in a follow-up patch after this.
sun’s picture

Title: Port Masquerade to D8 » Port Masquerade to D8 and rewrite + simplify it into a new 2.x series
Status: Needs review » Active

Committed #31.

@andypost: I'd recommend to wait with a 8.x-2.x release (D8 will still change under the hood anyway...). Please note that I do not plan to work on the masquerade_advanced sub-module. Instead, I'll work on the following topics next:

  1. Administration menu integration (as in #10).
  2. Toolbar integration (similar).
  3. Investigate possible + smart ways to generate a suggested list of users (zero-conf).
  4. Investigate possible + smart ways to disallow a lesser-privileged user to masquerade as a more privileged user (zero-conf).
  5. Moving some code into MasqueradeController (routing) and MasqueradeManager services.

That said, I'm done for today. ;) I first need to sleep over aforementioned topics either way. :)

deekayen’s picture

Status: Active » Needs work

You guys sure moved fast on this.

Reviewing whats been commited so far, I'm not clear whether the CSRF vulnerability has been re-introduced with the chopping. see http://drupal.org/node/835900

I see you've also maintained the masquerade as admin permission, which I want to kill. The permissions really need a full expansion of the grid so that each role can be allowed access to masquerade as one or more of the specific roles. If you're really going to continue with the "advanced" split, then the core module ought to have "masquerade as any user". The advanced part would allow the role breakout.

andypost’s picture

@deekayen CRSF protection still there and also covered with tests now.

About "admin permission" (restricted) - I still think we can get rid of it. Now there's a hook_masquerade_access() which allows to implement "advanced grid" of rights which makes "restricted" is obviously useless

sun’s picture

Status: Needs work » Active

@deekayen:

  1. @andypost is right, the CRSF protection still exists, and is actively covered by tests now.
  2. Any additional or more granular user permissions would have to go into the masquerade_advanced.module. The add-on module is not functional yet though. Does anyone want to work on #1926876: Fix and rework the masquerade_advanced sub-module?
  3. In general, I've started to document the architectural design and vision of the main masquerade.module in its README.txt for later reference.
  4. I'll continue to work on #32 + #10 + #12 this week. However, I have some other pressing $dayjob work to cater for, so it might take until next weekend for the implementations to fully work.
sun’s picture

FWIW, I pushed some initial changes for admin_menu 8.x-3.x to mute its existing Devel switch user integration in favor of Masquerade (if existent).

I've created a new feature branch 8.x-2.x-admin-menu, which already holds some basic (and working) integration code - mostly resembling the old switch user links (for now). Totally incomplete, and absolutely not fancy yet, but as said, at least it works (to some extent) already ;)

Not sure about the autocomplete form widget right within the menu yet, but that's exactly the smart and deep integration stuff we want to figure out and get right.

So if anyone feels eager to test this and wants to share UX feedback and ideas, please go ahead :)

(I just tried simplytest.me, but sadly, it did not pick up and provide the additional admin_menu module. Will try to notify @patrickd)

EDIT: Simplytest.me issue: #1927350: Additional module not downloaded in D8

moshe weitzman’s picture

I haven't tried it yet, but it sounds like Masquerade 8 will be simple and strong. When masquerade is released, I'll remove devel's switch user feature.

sun’s picture

Thanks for chiming in, @moshe! Your offer to drop the Switch user feature from Devel is one more awesome incentive to make the new Masquerade kick ass :-)

EDIT: Background: #49398: Integrate in devel.module ? :)


I'm a bit swamped with $dayjob work currently, but I'm still tinkering about #32. So far, my thoughts mainly revolved around the question of "smart permissions" — here's a braindump thus far:

  1. The built-in default masquerade access mechanism should be both trivially simple, but also guaranteed to be secure by default.
  2. KISS: We only want a single 'masquerade' permission; not an insane amount of (roles - 1)² permissions.
  3. Trivial: An authenticated user without any additional roles may only masquerade as another authenticated user that equally has no additional roles.
  4. Simple: A user with roles 'authenticated' + 'foo' + 'bar' may masquerade as any other user that has all of these roles or a smaller subset of these roles. The user may not masquerade as another user who has these roles and the additional 'baz' role.
  5. Advanced: Given two users, User A with roles 'foo' + 'bar' and User B with roles 'bar' + 'baz', there's no clear vertical intersection between the two. We could allow this case, but only if the additional role of the target user does NOT contain any permissions that are tagged with 'restrict access' => TRUE in hook_permission(). But I'm not 100% convinced of that.
  6. Utopian: Source and target user do not share any roles at all. Drupal's current state of access/permission system makes it impossible to determine whether masquerading as the target user would unexpectedly grant superpowers. Therefore, access has to be denied.

That said, I do believe that 3) + 4) cover the >80% case already. That is, because users, who are typically allowed to masquerade on a site, have higher level access permissions in the first place. In a most basic Drupal setup, that typically means users with the "administrator" role.

While I talked about roles in the above, it is possible that we want to base these security checks on the actual permissions of the associated roles instead. In particular, I'm thinking of the aforementioned "administrator" role, and that many sites are only granting this role to individual users - despite there being other roles - because this single role grants access to everything already. The same situation may arise when a site is not actually using the "administrator" role concept. In both cases, the list of roles is not meaningful. Only the difference/intersection of actual user permissions is the ultimate measure.

moshe weitzman’s picture

I don't have real world experience with masquerade, but a quick ponder leads me to these simple rules:

1. Admin choose which roles are blacklisted. These are roles whose users are can't be switched into. Defaults to the uber Administator role
2. uid1 is always ineligible to be switched into.
3. Users with the single 'Use masquerade' permission can switch into any user account which is eligible (i.e. not blacklisted).

sun’s picture

Yeah,

2. is already the case; i.e., the default access mechanism does not allow anyone to masquerade as uid 1.
3. A single 'masquerade' user permission is implemented already. What doesn't exist is a blacklist condition. In fact, it existed before and we just removed that. ;)

And that sorta gets us to the point of "smart default permissions": As originally mentioned in #12, the higher level goal is to make Masquerade a no-brainer to set up:

A slightly funny and interesting aspect is that I'm essentially entering this from a Devel module perspective, which apparently is that simple to set up already: Tick a single permission, done.

In using Devel's Switch user functionality over the past ~5+ years (dunno when it got introduced), this always played an important factor in site + module setup processes: Don't make me think. Just enable it, tick that thing, ready to rumble.

Even more importantly, it did not matter whether I had an actual need for the functionality. And I think that's what made Devel's Switch user feature so popular: It is really trivial to set up, and thus it is there when you need (if you need it). The DX and UX consequences of that are huge.

So if technically possible in any way, I'd like to implant some smart and intelligent access logic, which allows us to 1) retain KISS with a single permission, 2) ensures (sane) security by default, and 3) doesn't make me think.

In particular, I'd like to get away without any configuration page or settings form, and also without a flood of (n-1)^2 permissions. ;)

Alas, there are two conditions (#38.3 + .4) that could be implemented very easily. :) The only point at which it gets complex is when the roles/permissions between source and target user do not intersect vertically, but horizontally instead.

That said, we could always decide to support those simple 3 + 4 cases only, and leave any more complex access rules to the masquerade_advanced.module (which can happily ship with a flood of permissions and also a dozen of configuration pages; doesn't matter — or rather, that's why it exists in the first place). That is, because again, our focus should be on serving and solving the >80% use-case.

sun’s picture

Status: Active » Needs review
FileSize
18.76 KB

we could always decide to support those simple 3 + 4 cases only [...] our focus should be on serving and solving the >80% use-case.

So... I actually think that this is the way to go.

More granular access permissions like #1171500: Add "masquerade as @role" permissions/settings for each role can be happily implemented by an add-on module, which may completely override the default access implementation (or may also choose to extend it instead). We can leverage Drupal's modular design exactly in the way it is supposed to be used: 1) Simple baseline + 2) Optional enhancement.

Also, FWIW, I've studied the scenarios and use-cases that have been mentioned over in that issue. Some of them are covered with this simple access mechanism already. Only those that need super fine-grained access control (role-per-role, per-user, blacklist) would need the add-on module.

Attached patch implements this simple access mechanism, as outlined before, in this exact order of conditions:

  1. Early-return: Uid 1 can masquerade as anyone.
  2. Early-return: No one can masquerade as uid 1.
  3. If you have the identical roles as the target user (or additional roles), you are allowed to masquerade.
  4. If you have the identical permissions as the target user (or additional permissions), you are allowed to masquerade.
  5. Otherwise, access is denied.

This patch comes with full test coverage for all permutations and test cases. Most noteworthy:

A) The typical 'administrator' role (with all permissions).
B) A 'moderator' role (with typical moderation + masquerade permissions).
C) An 'editor' role (with typical editor + masquerade permissions).
D) A 'masquerade' role (just that).

And it positively confirms this test plan:

   * - root » admin
   * - admin ! root
   * - admin » moderator (more roles but less privileges)
   * - admin » masquerade (different role)
   * - admin » auth (less roles)
   * - moderator ! root
   * - moderator ! admin (less roles but more privileges)
   * - moderator ! editor (different roles + privileges)
   * - moderator » masquerade (less roles)
   * - moderator » auth
   * - [editor is access-logic-wise equal to moderator, so skipped]
   * - masquerade ! root
   * - masquerade ! admin (different role with more privileges)
   * - masquerade ! moderator (more roles)
   * - masquerade » auth
   * - masquerade ! masquerade (self)
   * - auth ! *

In turn, we get away with a single user permission that is secure, simple, and smart, and we solve the >80% use-case.

Note that the patch is a little confusing due to renames=copies. I've pushed this code as 8.x-2.x-access feature branch.

sun’s picture

Status: Needs review » Active

Sleeping over this, I'm still 100% confident that this is the most optimal way forward.

Thus, committed (with a small comment typo fix) to 8.x-2.x.

sun’s picture

FileSize
2.3 KB

Also committed docs and help text for the new built-in access control mechanism.

deekayen’s picture

I've been using Masquerade happily on production sites for years, but I think #41 turns it into a development-only module.

Take a site that has various companies or vendors who can login and place orders. They're assigned a Vendor role. I wouldn't want any of those vendors to login as a competing company's user and place fraudulent orders. Sure, they have the same access, but that doesn't mean that just because they want to masquerade as a user of the same role, that they should be allowed.

I understand you're trying to make this something that you can just turn on and it "works" without additional configuration, but without doing #1171500: Add "masquerade as @role" permissions/settings for each role alongside this change, you're breaking a lot of years of backwards functional expectation and compatibility across thousands of sites.

sun’s picture

Sure, I perfectly understand that there are advanced use-cases that require some more sophisticated access control mechanisms. But as mentioned before, the built-in default access control can be swapped out and/or enhanced easily.

A masquerade_advanced/masquerade_access [sub-?]module can provide these more granular access options. If done right, then this add-on module is equally small and simple, and solely focuses on solving exactly those advanced access requirements in the most clean and best way. For your particular use-cases, that's just going to be a click away to enable the add-on module.

Consequently, this is not a question of doing one thing OR another thing. — Instead, it's about sane defaults, vertical extensibility, and separation of concerns. In other words, we can easily have both, and no one is going to suffer.

The Masquerade module exposes a hook_masquerade_access() already, which follows the hook_node_access() architecture. This hook allows modules to grant/deny masquerade access based on other/custom access rules. If desired, the built-in default hook implementation can even be eliminated entirely via hook_module_implements_alter().

That said, I'm also happy to discuss the option of turning the access mechanism into a registered service instead of a hook. This would mean that there can only be one access implementation, and the default implementation can only be swapped out entirely with another, but not with multiple — which may or may not make sense.

In short, all of this is still possible, and we actually kept the need for those advanced use-cases in mind while revamping the code. The one and only difference is that these advanced options are no longer provided by the main module.

I'd really like to encourage you and all others to discuss which advanced real world use-cases should be covered by the add-on module, and how they should be ideally implemented from a UX and feature perspective. We have the following issues for that:

#1926876: Fix and rework the masquerade_advanced sub-module
#1171500: Add "masquerade as @role" permissions/settings for each role

The latter is a concrete implementation proposal already, but I actually think it would make a lot of sense to have a higher-level discussion about use-cases, concepts, and requirements first. That is, because the total combination of advanced access rules — role-per-role permissions, user-specific grants to other users/roles, global blacklist/whitelist, etc — involve plenty of options and conditions already, and the resulting cumulative set of access rules as well as their UX will be non-trivial to figure out for site maintainers. In turn, that also presents a security risk.

Speaking of security, if that advanced access implementation wants to live in the main Masquerade project, it will need 100% test coverage. If that's not going to be the case, then the module should live in a separate d.o project, since we'd not be able to make security guarantees otherwise.

deekayen’s picture

Speaking in the context of your desire to get this added to core, I'm speaking to the darks side of your proposal, for the uninformed admins that will use the module.

I've made the case before, specifically about OpenID, that site admins will enable modules and have no idea what they do, let alone their related security implications. What we're talking about with your proposed default configuration for permissions is giving admins a module in core, which if enabled and not configured, turns the site into a development platform. It would grant to users in a forum the rights to impersonate another user during a flamewar, give vendors in a commerce site to place fake orders for a competitor, give employees the rights to delete their boss's account, give peers the rights to pretend to do code review in Open Atrium style sites, and so on.

Contributed modules will see Masquerade as a possible dependency. They'll require it to be enabled for some silly use of a single function that really has nothing to do with masquerading as another user, but then the site will have all the relevant impacts.

I think you're trying to make security easy, but security sucks. If on the other hand you were proposing to make #1171500: Add "masquerade as @role" permissions/settings for each role the default and have a addon module called masquerade_devel with your proposed permissions, that seems like it would make more sense to me.

tecto’s picture

Latecomer to this conversation, but speaking from an information security perspective, the Masquerade module (in the current incarnation) falls into the category of a necessary evil. Masquerade violates a primary rule of information security: accountability. Users cannot be held accountable for their actions when those actions cannot be traced back to them. Masquerade exists because in spite of this violation, there are scenarios (testing during development, user support on live sites) that are significantly challenging without this option.

To compensate for this violation, it is appropriate to narrowly control the ability to masquerade as another user. Security is hard. The law of unintended consequences bites in this area with surprising frequency and cost. If we are going to knowingly violate a cardinal rule of information security, we should not mask the reality that we are very likely to introduce a security vulnerability.

An automatic grant of masquerade rights for any account that has matching or fewer permissions than the currently logged in user makes sense in a development (test/staging) environment. To distinguish the appropriate use, that functionality should be part of something like masquerade_devel.

Making the stock masquerade module function this way is a recipe for a site admin to enable the module for a specific need and not realize the fact that he has granted other users extraordinary access.

While a permissions page that looks like a wall of checkboxes is intimidating, it is appropriate. Just as the workflow module creates a wall of options as the workflow states and roles expand, permissions for masquerading appropriately grows more intimidating as the complexity of the many roles in a complex site grows. It conveys that the decisions to be made are non-trivial and should be considered carefully.

liquidcms’s picture

also late to this and sad to see Masquerade being neutered (as Admin Role was when feebly pulled in to core for D7). The way Masquerade works now (D7) is great. Just because 1 or 2 guys don't see need for a feature like masq as anon; doesn't mean it isn't useful.

I am all for simplifying code; but certainly not for reducing functionality.

Hoping we don't end up needing a Masquerade 2 to replace the lost functionality (like I had to do with AdminRole).

andypost’s picture

alexander_danilenko’s picture

Status: Active » Needs review
FileSize
1 KB

Hi guys!

When i place Masquerade block (/admin/structure/block/add/masquerade/bartik) i get an error

Fatal error: Class Drupal\masquerade\Plugin\Block\MasqueradeBlock contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\block\BlockPluginInterface::build) in
\modules\masquerade\lib\Drupal\masquerade\Plugin\Block\MasqueradeBlock.php on line 24

It seems we have to use build()method instead blockBuild().
It resolves error for me and block with user switcher form appears on page.

Review patch please.

andypost’s picture

alexander_danilenko’s picture

FileSize
10.27 KB

Hello, guys! New patch due to new changes in Drupal 8 core (:

  • /masquerade page form implemented as class that implements FormInterface
  • /masquerade page moved to route layer
  • Some deprecated functions replaced
  • All $user->uid replaced with $user->id() (only in masquerade.module)
  • Removed function drupal_goto() replaced with RedirectResponse object

Review please ;)

andypost’s picture

Assigned: sun » Unassigned

Great step forward. but still needs work

+++ b/lib/Drupal/masquerade/Form/MasqueradeBlockForm.phpundefined
@@ -0,0 +1,90 @@
+  protected $database;
...
+    return new static($container);
...
+  public function __construct(Connection $database) {
+    $this->database = $database;

$database needs proper injection. Your code passes container as a whole.
I'd recommend you to check core 8 for examples of this kind of injection

+++ b/masquerade.moduleundefined
@@ -63,12 +64,9 @@ function masquerade_permission() {
   $items['masquerade'] = array(
...
+    'title' => 'Masquerade',
+    'route_name' => 'masquerade',

This is a callback so not needed at all. just remove

+++ b/masquerade.moduleundefined
@@ -349,10 +297,10 @@ function masquerade_switch_user_validate(User $target_account) {
+  if (Drupal::config('system.maintenance')->get('enabled') && !user_access('access site in maintenance mode', $target_account)) {

user_access() is deprecated. use $target_account->has_permission() instead.

+++ b/masquerade.moduleundefined
@@ -414,7 +362,10 @@ function masquerade_switch_back_page() {
+    $redirect = new \Symfony\Component\HttpFoundation\RedirectResponse(url($url, array('absolute' => true)));
+    $redirect->send();

suppose you just need to return $response

+++ b/masquerade.routing.ymlundefined
@@ -2,8 +2,15 @@
-  pattern: '/masquerade/autocomplete'
+  pattern: 'masquerade/autocomplete'

this change is not needed

+++ b/masquerade.routing.ymlundefined
@@ -2,8 +2,15 @@
\ No newline at end of file

please, add new line

andypost’s picture

FileSize
20.66 KB

Commited a bunch of fixes

PS: tests still broken

andypost’s picture

Pushed 2 commits, module now installs but CSRF menu alters does not works.
Form moved to own file
Some code clean-ups

  • Commit 130815d on 8.x-2.x by sun:
    - #1836516 by sun: Added docs and help text for new access control...
  • Commit 209054a on 8.x-2.x, 8.x-2.x-admin-menu by sun:
    - #1836516 by sun: Revamped masquerade access and UI validation.
    
  • Commit 2472573 on 8.x-2.x, 8.x-2.x-admin-menu by sun:
    - #1836516 by sun: Added project documentation, goals and vision.
    
  • Commit 26029f8 on 8.x-2.x, 8.x-2.x-admin-menu by sun:
    - #1836516 by sun: Fixed tests via helper methods to work around...
  • Commit 48dc664 on 8.x-2.x, 8.x-2.x-admin-menu by sun:
    - #1836516 by sun: Restored Masquerade block.
    
  • Commit 558568e on 8.x-2.x, 8.x-2.x-admin-menu by sun:
    - #1836516 by sun: Added masquerade_user_is_masquerading() helper...
  • Commit 659a8b6 on 8.x-2.x, 8.x-2.x-admin-menu by sun:
    - #1836516 by sun: Moved advanced features into separate...
  • Commit 6c71dfb on 8.x-2.x, 8.x-2.x-admin-menu, 8.x-1.x-1836516 authored by sun, committed by andypost:
    Issue #1836516 by sun: Port Masquerade to D8
    
  • Commit 9275c3f on 8.x-2.x, 8.x-2.x-admin-menu by sun:
    - #1836516 by sun: Implemented new secure, simple, and smart user access...
  • Commit c50a383 on 8.x-2.x, 8.x-2.x-admin-menu by sun:
    - #1836516 by sun: Added API documentation, hook_help(), and cleaned up...
  • Commit fcfb10e on 8.x-2.x, 8.x-2.x-admin-menu by sun:
    - #1836516 by sun: Changed /masquerade/switch/% into /user/%user/...

  • Commit 6c71dfb on 8.x-2.x, 8.x-2.x-admin-menu, 8.x-1.x-1836516 authored by sun, committed by andypost:
    Issue #1836516 by sun: Port Masquerade to D8
    
andypost’s picture

Right now module mostly chasing HEAD

@todo is:
1) fix permissions
2) clean-up controlllers
3) extract switch to service helper class
4) add toolbar button and probably, modal form to switch
then
4) advanced module

I see this one as masquerade_ui or advanced that provides integration to third-party modules (action, condition, config form to blacklist roles, inject info user entity form/view, role form)

jhedstrom’s picture

This patch is focused on getting the tests to a usable state. There are some failures with this, but they actually look like legitimate fails, rather than the current fails that are due to fatal errors.

jhedstrom’s picture

FileSize
822 bytes
14.39 KB

The user.admin_account route was changed in latest Beta.

andypost’s picture

@jhedstrom are you able to run tests locally? module was broken in switching.
I can't test right now, but it seems makes sense to commit the patch to unfreeze testing, is not it?

jhedstrom’s picture

FileSize
29.07 KB

@andypost running locally with the patch in #60 is able to complete the test run, whereas without the patch, fatal errors and exceptions prevent completion. There are a few fails, but they look to be like legitimate fails, rather than bugs in the tests. Attached is the output from the command:

php core/scripts/run-tests.sh --verbose --color --concurrency 4 --php `which php` --url http://d8.devl "masquerade" | tee /tmp/test.txt; TEST_EXIT=${PIPESTATUS[0]}; echo $TEST_EXIT

  • andypost committed 9996a43 on 8.x-2.x authored by jhedstrom
    Issue #1836516 by jhedstrom: Port Masquerade to D8 - initial fix for...
andypost’s picture

@jhedstrom thanx, commited #60, that's much better then previous state, we need to unblock testing patches

  • andypost committed a45a7bd on 8.x-2.x
    Issue #1836516 by andypost: Fix extra fields
    
andypost’s picture

FileSize
27.33 KB

Implemented Masquerade service and cleaned-up procedural functions
@todo Remove masquerade_switch_user_validate()

  • andypost committed aa0e8e7 on 8.x-2.x
    Issue #1836516 by andypost: Fix admin_role https://www.drupal.org/node/...
andypost’s picture

Suppose to close the issue once access/validation logic from masquerade_switch_user_validate() will find it's home.

Now a list of follow-ups

  • andypost committed 4d71a5c on
    Issue #1836516 by andypost: Fix autocomplete https://www.drupal.org/node...
andypost’s picture

Fixed tests in MasqueradeTest - csrf seed is not accessible from parent site, test using clickLink()
Also fixed autocomplete and redirect.

Somehow "Unmasquerade" link is always visible, needs work as well as MasqueradeAccessTest

Francewhoa’s picture

Thanks all for your contributions

We're happy to contribute testing patch, quality assurance, documentation, and agile project management services if needed

Related pages

  • andypost committed b13b3fa on 8.x-2.x
    Issue #1836516 by andypost: Remove deprecated funstion usage from module...

  • andypost committed b03d01b on 8.x-2.x
    Issue #1836516 by andypost: Fix usage of placeholders in hook_help()
    

  • andypost committed c5175a5 on 8.x-2.x
    Issue #1836516 be andypost: Fix placeholders and username functions
    

  • andypost committed a190821 on 8.x-2.x
    Issue #1836516 be andypost: Fix hook_ENTITY_TYPE_view() https://www....

  • andypost committed aa352f6 on 8.x-2.x
    Issue #1836516 be andypost: Use getDisplayName() intead deprecated...
andypost’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.