Latest version of bakery -> http://drupal.org/project/bakery breaks masquerade switch functionality with this check for cookies -> http://drupalcode.org/project/bakery.git/blobdiff/fe89310560f91374f0bb59... . Solution was to clear bakery cookies and set a new cookie for the new user. Patch below

CommentFileSizeAuthor
#9 1364574.patch1.91 KBdrumm
#7 masquerade_login_logout_hooks-1364574-7.patch1.57 KBartkon
#5 login_logout_hooks-1364574-1.patch902 bytesartkon
#3 bakery_support_masquerade-1364574-1.patch1008 bytesartkon
#2 1180546-block-masquerade-3.patch2.49 KBartkon
#1 bakery_support_masquerade-1364574-1.patch1009 bytesartkon

Comments

artkon’s picture

artkon commented
StatusFileSize
new bakery_support_masquerade-1364574-1.patch1009 bytes
artkon’s picture

artkon commented
StatusFileSize
new 1180546-block-masquerade-3.patch2.49 KB

Last patch had a mistake, this one is good.

artkon’s picture

artkon commented
StatusFileSize
new bakery_support_masquerade-1364574-1.patch1008 bytes
andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 6.x-1.4 » 6.x-1.x-dev

Any ideas to test this? Suppose we should wait for someone using this modules together

artkon’s picture

artkon commented
StatusFileSize
new login_logout_hooks-1364574-1.patch902 bytes

Might consider this patch useful not only for bakery module but in general would think we need to call all the hooks of user_login and user_logout.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Probably you right! Changing the active user should invoke hooks

artkon’s picture

artkon commented
Version: 6.x-1.x-dev » 6.x-1.7
StatusFileSize
new masquerade_login_logout_hooks-1364574-7.patch1.57 KB

Just updated to new tag 6.x-1.7 and created patch. One thing to note is that I had to use sess_regenerate() after calling the hooks for logout because bakery module deletes the session. Core calls this function when switching from anonymous to new user in user_authenticate_finalize() so seems consistent.

sheldonkreger’s picture

sheldonkreger commented

My team recently ran a Bakery upgrade on a D7 site and lost Masquerade functionality. I suspect this is a similar issue. Any thoughts?

drumm’s picture

drumm
he/him
Location NY, US
commented
Version: 6.x-1.7 » 7.x-1.x-dev
Status: Active » Needs work
StatusFileSize
new 1364574.patch1.91 KB

This is an issue with 7.x too. I tried the attached straightforward update of #7, but it isn't enough. I'm guessing the issue is that we don't give out the 'bypass bakery' permission, so the two modules are in conflict. I'll have to investigate more.

Regardless, invoking hooks and regenerating session cookies are a great thing to do. Minor nit- the $edit parameter is usually an array, so passing an empty array instead of NULL has less of a chance of confusing other modules.

drumm’s picture

drumm
he/him
Location NY, US
commented
Title: Latest version of bakery module breaks masquerade functionality. » Invoke logout & login hooks for compatibility with bakery
Assigned: Unassigned » drumm
Status: Needs work » Needs review

I've confirmed this works when the bakery slave has #1599522: Compatibility with masquerade on Drupal.org dev sites.

deekayen’s picture

deekayen commented

I guess what I'm concerned about is not really the code, but the philosophical implementation. Historically, just because you're masquerading doesn't really mean you're logging out, where this patch would make a point of killing the original session. This is not really a "bug fix" as I see it, but a functionality change. Is this worthy of a 7.x-2.x?

drumm’s picture

drumm
he/him
Location NY, US
commented

Changing the session ID when privileges change is a good best practice, mitigating session fixation and hijacking attacks.

Other approaches I can imagine are:
- Archive the user's cookies, replacing them with a new set when masquerading starts, and restoring them when it ends.
- Disable or change the logic invoked by bakery_boot() to be more lenient.

deekayen’s picture

deekayen commented
Status: Needs review » Fixed

After reviewing some other issues in the queue, I settled on session regeneration being a good idea and committed #9.

Status: Fixed » Closed (fixed)

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

kenianbei’s picture

kenianbei commented

I just found that this commit prevents shibboleth users from using the masquerade function, as it logs them out.

I've only tested this on one of my production sites, when I get time I will test it on a fresh install.

andypost’s picture

andypost
he/him
Primary language Russian
commented

@kenianbei Does it works for you?

neclimdul’s picture

neclimdul
He/Him
commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Issue summary: View changes
Status: Closed (fixed) » Patch (to be ported)

Can we apply #1364574-7: Invoke logout & login hooks for compatibility with bakery to 6.x? Just tested and it still applies and I've been running this patch for over a year.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Patch (to be ported) » Reviewed & tested by the community

sorry, meant to rtbc #7

The last submitted patch, 1: bakery_support_masquerade-1364574-1.patch, failed testing.

The last submitted patch, 2: 1180546-block-masquerade-3.patch, failed testing.

The last submitted patch, 3: bakery_support_masquerade-1364574-1.patch, failed testing.

The last submitted patch, 5: login_logout_hooks-1364574-1.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 1364574.patch, failed testing.

drumm’s picture

drumm
he/him
Location NY, US
commented
Assigned: drumm » Unassigned
Status: Needs work » Reviewed & tested by the community
drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Needs work

I guess this should be reset.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review

grumble... it was testing patches other then 7. Think 7 passed. I think putting it back NR should get it tested again.

  • Commit 2d1eac1 on 7.x-1.x, 8.x-2.x, 8.x-2.x-admin-menu, 8.x-1.x-1836516 authored by drumm, committed by deekayen: 
    Issue #1364574 by drumm, artkon, sheldonkreger: Invoke logout...

  • Commit 2d1eac1 on 7.x-1.x, 8.x-2.x, 8.x-2.x-admin-menu, 8.x-1.x-1836516 authored by drumm, committed by deekayen: 
    Issue #1364574 by drumm, artkon, sheldonkreger: Invoke logout...
ericras’s picture

ericras commented
Related issues: +#2568789: Add admin-setting to disable calls to hook_user_logout
ericras’s picture

ericras commented
Related issues: +#2896370: Masquerade with LDAP Single Sign On
ericras’s picture

ericras commented
Related issues: +#1813696: Set/unset Session flag when masquerade status changes.
ericras’s picture

ericras commented

This change (that was included in 7.x-1.0-rc5) broke use with SSO solutions like LDAP and Shib that invoke user_logout to actually logout from (and possibly redirect to) an external auth provider.

ericras’s picture

ericras commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
ressa’s picture

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

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

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

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

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

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

Maintainers, credit people who helped resolve this issue.