Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | masquerade_login_logout_hooks-1364574-7.patch | 1.57 KB | artkon |
| #1 | bakery_support_masquerade-1364574-1.patch | 1009 bytes | artkon |
Comments
Comment #1
artkon CreditAttribution: artkon commentedComment #2
artkon CreditAttribution: artkon commentedLast patch had a mistake, this one is good.
Comment #3
artkon CreditAttribution: artkon commentedComment #4
andypostAny ideas to test this? Suppose we should wait for someone using this modules together
Comment #5
artkon CreditAttribution: artkon commentedMight 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.
Comment #6
andypostProbably you right! Changing the active user should invoke hooks
Comment #7
artkon CreditAttribution: artkon commentedJust 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.
Comment #8
sheldonkreger CreditAttribution: sheldonkreger commentedMy team recently ran a Bakery upgrade on a D7 site and lost Masquerade functionality. I suspect this is a similar issue. Any thoughts?
Comment #9
drummThis 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.
Comment #10
drummI've confirmed this works when the bakery slave has #1599522: Compatibility with masquerade on Drupal.org dev sites.
Comment #11
deekayen CreditAttribution: deekayen commentedI 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?
Comment #12
drummChanging 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.
Comment #13
deekayen CreditAttribution: deekayen commentedAfter reviewing some other issues in the queue, I settled on session regeneration being a good idea and committed #9.
Comment #15
kenianbei CreditAttribution: kenianbei commentedI 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.
Comment #16
andypost@kenianbei Does it works for you?
Comment #17
neclimdulCan 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.
Comment #18
neclimdulsorry, meant to rtbc #7
Comment #24
drummComment #25
drummI guess this should be reset.
Comment #26
neclimdulgrumble... it was testing patches other then 7. Think 7 passed. I think putting it back NR should get it tested again.
Comment #29
ericras CreditAttribution: ericras at University of Nebraska commentedComment #30
ericras CreditAttribution: ericras at University of Nebraska commentedComment #31
ericras CreditAttribution: ericras at University of Nebraska commentedComment #32
ericras CreditAttribution: ericras at University of Nebraska commentedThis 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.
Comment #33
ericras CreditAttribution: ericras at University of Nebraska commented