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

Files: 
CommentFileSizeAuthor
#7 masquerade_login_logout_hooks-1364574-7.patch1.57 KBartkon
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#1 bakery_support_masquerade-1364574-1.patch1009 bytesartkon
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch bakery_support_masquerade-1364574-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

artkon’s picture

StatusFileSize
new1009 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch bakery_support_masquerade-1364574-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
artkon’s picture

StatusFileSize
new2.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1180546-block-masquerade-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Last patch had a mistake, this one is good.

artkon’s picture

StatusFileSize
new1008 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch bakery_support_masquerade-1364574-1_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
andypost’s picture

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

StatusFileSize
new902 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch login_logout_hooks-1364574-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Probably you right! Changing the active user should invoke hooks

artkon’s picture

Version:6.x-1.x-dev» 6.x-1.7
StatusFileSize
new1.57 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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

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

Version:6.x-1.7» 7.x-1.x-dev
Status:Active» Needs work
StatusFileSize
new1.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1364574.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

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

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

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

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

@kenianbei Does it works for you?

neclimdul’s picture

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

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

Assigned:drumm» Unassigned
Status:Needs work» Reviewed & tested by the community
drumm’s picture

Status:Reviewed & tested by the community» Needs work

I guess this should be reset.

neclimdul’s picture

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...