If you are supporting SSO with Bakery (or similar code), masquerade will not work. The masqueraded user will be denied access because the persistent id info (Cookie in Bakery's case) will not match.

These actions generally take place when hook_boot() is called.

It is hard to add code to a hook_boot() implementation that identifies a masquerade user switch because the session flag is only set in the masquerade_init() process... after the boot() process. In other words:

  • Masquerade user is selected
  • Next page loaded is loaded: SSO Boot process checks for $_SESSION[masquerade], doesn't find it and denies user
  • Masquerade_init() not called to set session flag

Currently, IFAIK, the only work around for this is to modify your SSO code to duplicate the masquerade init code and double check for masquerading users. Very inefficient since the DB calls are made twice... and you may need to query the system table to see if masquerade is installed.

If masquerade_switch_user0 where to set the session flag and masquerade_switch_back() unset it, the the session flag could be used by boot() implementations to manage masqueraded users. This would be much more efficient since no DB calls would be needed... e.g. just do something like: 

if ( isset($_SESSION['masquerade']) ) 
    // don't check persistent data

An alternative to this would be to add a couple of masquerade API hooks that get called with users are switched. This would allow implementations to do any additional user setup / teardown needed by other modules.

CommentFileSizeAuthor
#1 masquerade-6.x-allow_SSO_boot_to_identify_masquerading_user-1813696-1.patch1.28 KBcgmonroe
#1 masquerade-7.x-allow_SSO_boot_to_identify_masquerading_user-1813696-1.patch1 KBcgmonroe
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cgmonroe’s picture

cgmonroe CreditAttribution: cgmonroe commented
FileSize
masquerade-7.x-allow_SSO_boot_to_identify_masquerading_user-1813696-1.patch1 KB
masquerade-6.x-allow_SSO_boot_to_identify_masquerading_user-1813696-1.patch1.28 KB

Here are patches for the current git 6.x and 7.x branches that add setting/unsetting the session flag when a user change is made.

cgmonroe’s picture

cgmonroe CreditAttribution: cgmonroe commented
Status: Active » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Looks like duplicate of commited #1364574: Invoke logout & login hooks for compatibility with bakery

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented 
+++ b/masquerade.module
@@ -846,6 +847,10 @@ function masquerade_switch_back() {
+  if ( isset($_SESSION) ) {

extra space within if()

cgmonroe’s picture

cgmonroe CreditAttribution: cgmonroe commented
Title: Allow SSO boot functions to identify a masquerading user » Set/unset Session flag when masquerade status changes.
Version: 6.x-1.x-dev » 7.x-1.x-dev

Re: Duplicate issue

Yes, the use case illustrating these issues are duplicate but the underlying issues are different. This is because the problem that lead me to creating this issue was caused by a similar use case with some custom SSO code. So my use case used to describe the need / issue was a duplicate of the other issue's use case.

However, IMHO, delaying setting the session flag until the next page load init call is a separate bug. Downstream developers should be able to depend on the session flag being set at all times during the masquerade process. SSO is a single use case that caught this, but I'm sure there are others.

I have changed the title to more clearly reflect the larger issue... and upped the version to 7.x... since it effects both 6 and 7.

To restate the problem:

The $_SESSION['masquerading'] property (or lack of it) is missing during the start and end masquerade periods because the flag is not set until the next page's init() call. This means that it is not available to some hook implementation that might need it. E.g., hook_boot() on the page after masquerading starts and hook_exit() on page ends.

Re: space?

Not sure of the problem here... the patched module passes Coder with report minor problems set (well some other parts of the masquerade code fails, but not my patched code). AFAIK the Drupal coding standard only requires a space between if and the starting paren of the logicial. It does not define the layout of the logical except to keep it readable and under 80 chars if possible.

I'll rebuild the patch if this is a masquerade coding requirement.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Is there any other way to fill $_SESSION['masquerading'] to make it accessible at hook_boot/init()
We lack of tests so this patch needs deeper review and probably refactoring the logic of using the setting

PS: np, about spaces...

cgmonroe’s picture

cgmonroe CreditAttribution: cgmonroe commented

Hmm, the logic doesn't seem to complex...

The session flag is used to determine what link to show the user. E.g., non-masquerading users do not have the session flag and see "Switch to User" text. Masquerading users have the flag and this is used to determine that the "Switch back to..." text. is displayed.

The current logic

Start Masquerading Page:

  • User selects one of the "Masquerade as options"
  • All options call masquerade_switch_user()
  • $_SESSION['masquerading'] used to prevent recursive masquerading
  • This sets up the masquerading stuff and inserts a record in the masquerade table.
  • Drupal go to called to send to a new page.
  • hook_exit called (Session not set yet)

All "Masquerading as" Page loads:

  • hook_boot called (PROBLEM: $_Session['masquerading'] not set on initial masquerading as page load)
  • masquerade_init() called on page load
  • does a DB query to masquerade table to get the original id
  • sets the $_SESSION['masquerade'] flag
  • Other page logic uses $_SESSION['masquerading'] to modify links.
  • hook_exit called (Session set)

Stop Masquerading page

  • User selects to switch back
  • masquerade_switch_back() called
  • Masquerading session info cleared from the masquerade DB table
  • user info swapped back
  • drupal go to called
  • hook_exit called ( PROBLEM: $_SESSION['masquerading'] is still set )

Normal User Page load

  • hook_boot called (PROBLEM: $_Session['masquerading'] is set on initial post masquerading swich page load)
  • masquerade_init() called on page load
  • does a DB query to masquerade table to get the original id - Not found
  • unsets the $_SESSION['masquerade'] flag if needed.
  • Other page logic uses the fact that $_SESSION['masquerading'] is not set to modify links.
  • hook_exit called

The modified logic

Start Masquerading Page:

  • User selects one of the "Masquerade as options"
  • All options call masquerade_switch_user()
  • $_SESSION['masquerading'] used to prevent recursive masquerading
  • This sets up the masquerading stuff and inserts a record in the masquerade table.
  • NEW: sets the $_SESSION['masquerading'] flag
  • Drupal go to called to send to a new page.
  • hook_exit called (Session not set yet)

All "Masquerading as" Page loads:

  • hook_boot called (CORRECT BEHAVIOR: $_Session['masquerading'] IS SET on initial masquerading as page load)
  • masquerade_init() called on page load
  • does a DB query to masquerade table to get the original id
  • sets the $_SESSION['masquerading'] flag
  • Other page logic uses $_SESSION['masquerading'] to modify links.
  • hook_exit called (Session set)

Stop Masquerading page

  • User selects to switch back
  • masquerade_switch_back() called
  • Masquerading session info cleared from the masquerade DB table
  • user info swapped back
  • NEW: unsets the $_SESSION['masquerading'] flag if needed.
  • drupal go to called
  • hook_exit called ( CORRECT BEHAVIOR: $_SESSION['masquerading'] IS NOT SET )

Normal User Page load

  • hook_boot called (CORRECT BEHAVIOR: $_Session['masquerading'] IS NOT SET on initial post masquerading switch page load)
  • masquerade_init() called on page load
  • does a DB query to masquerade table to get the original id - Not found
  • unsets the $_SESSION['masquerading'] flag if needed.
  • Other page logic uses the fact that $_SESSION['masquerading'] is not set to modify links.
  • hook_exit called

This patch does not change the default behavior, just covers the entry/exit points better. Note that the additional session flag changes are occurring right before a drupal_goto() which ends the request processing with an exit call. So there should not be any conflict with other code.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Priority: Normal » Major

@cgmonroe, Thanx a lot for write-up. I suppose you mostly right but I need to test more the module.
I think we need to implement hook_boot() anyway to allow currently fragile masquerading as anonymous user.
It would be a great clean-up before stable release

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

There's a D8 version that relies only on $_SESSION #1926074: Remove {masquerade} table and rely on session flag only
After the test coverage it will be commited and should backported to 7 branch

Status: Needs review » Needs work

The last submitted patch, masquerade-7.x-allow_SSO_boot_to_identify_masquerading_user-1813696-1.patch, failed testing.

rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented

Another potentially related issue: #2226531: Login hooks can't detect masqueraded user with $_SESSION['masquerading']

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Related issues: +#1458418: Add masquerade events API

An alternative to this would be to add a couple of masquerade API hooks that get called with users are switched. This would allow implementations to do any additional user setup / teardown needed by other modules.

I'm linking this to an existing issue for that solution.