Masquerade as blocked user returns a 403 error and logs out the user

Adding updated #9 patch after adding changes which was suggested In #10 comment.

It should be $target_account->isBlocked(), not $target_account->status->isBlocked()

Still needs tests.

Also, what about this code in SwitchAccessCheck.php?

    elseif ($account->id() == 1) {
      // Uid 1 may masquerade as anyone.
      $result = AccessResult::allowed();
    }
    elseif ($account->isBlocked()) {
      $result = AccessResult::forbidden();
    }

Wouldn't user 1 still be logged out and have issues when masquerading as a blocked user account? Should the isBlocked check come first?

This fixes the tests in MasqueradeAccessTest.

Related: 📌 Extend test coverage to note when the pages should give 200 or 403 errors Fixed

+++ b/src/Access/SwitchAccessCheck.php
@@ -39,6 +39,7 @@ public function __construct(Masquerade $masquerade) {
+    // @todo Should anonymous users be blocked outright?

yes, for anomymous user there's browser's incognito mode

Some small improvements.

Is there anything that could be done to improve the test coverage for this scenario?

Let's use linkExists('Unmasqueraade') instead of checking for just text on the page.

The last submitted patch, 20: masquerade-n3258364-20.patch, failed testing. View results →

The issue with the patch is that you spelled "Unmasquerade" wrong.

*facepalm*

Let's try that again, shall we.

It looks good to me, but I'm not comfortable counting my review as RTBC, as I can't reproduce the issue myself.

Going to add some test coverage for a blocked user.

Some test coverage for the blocked user account. I also removed the special case that was added for user 1, that should be handled by the permission system instead of having magic logic for a specific user ID.

The special logic for user 1 will be handled in a separate issue: ✨ Remove special logic for user 1 in SwitchAccessCheck RTBC .

This interdiff is against #23, just pretend #26 doesn't exist ;-)

I'm not sure about doing a tests-only patch as the tests depend upon some verbiage that only shows with the new changes. I suppose we could work out a different way of confirming the user cannot log in as a blocked user?

Blocked users!
Nice catch - now it's more clear why this happening randomly

Sadly we can't catch moment when user is blocked while masquerading,
need to check how it works on vanilla core - I bet session just failed to load user.

Probably all places should exclude blocked

I tried comparing $this->loggedInUser->getAccountName() to $target_account->getAccountName() but the loggedInUser variable isn't changed when masquerading.

Going through the logic I found that the autocomplete field only returns blocked users if you have the "administer users" permission. Because of this, users who do not have that permission cannot log in as a blocked user because core blocks them, while users who do have the permission could log in as the blocked user but the code changes to masquerade.module block it.

Some additional bits to make sure the correct user is logged in after doing the masquerade.

I have applied Patch #31 to Masquerade 8.x-2.0-rc4 and my installation of Drupal 9.5.10. That patch effectively disables the "masquerade as" menu option in for blocked users, a feature of significant importance for me. Additionally, this patch prevents users from masquerading as themselves and from masquerading as other users who are already in a masqueraded state. I have conducted extensive testing to verify the reliability of this patch.

Based on my experience and testing, I believe that this fix is ready for use and should be incorporated into the module's codebase. It addresses a critical issue and enhances the module's functionality for a broader user base.

The last submitted patch, 11: 3258364-11.patch, failed testing. View results →

Was there an issue that occurs when a user masquerades as someone who is currently masquerading?

I think the changes should be applied to the MR, to avoid confusion. The issue has an open merge request, so changes should not have been made by submitting patches.