Closed (fixed)
Project:
Masquerade
Version:
8.x-2.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
24 May 2018 at 18:32 UTC
Updated:
22 Jan 2020 at 23:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
jnicola commentedAdding in a patch to comment out
$this->moduleHandler->invokeAll('user_logout', [$account]);but still executedrupal_static_reset('template_preprocess, which is called in user_user_logout and should be called.For the implementation we are using no other contrib modules are using this hook besides the core user module, so this should be all good.
Comment #3
berdir> In D8, the proposed D7 solution does not get us there.
I think it would because this is the same, except there it's configurable and this is hardcoded which obviously doesn't work as a patch that could be committed.
However, the maintainer in the other issue was against this approach. I'm not 100% sure I get that, though. Wondering about what the use cases are for calling the logout hook. There seem to be very few implements of that out there.
Maybe we could alternatively move $_SESSION['masquerading'] = $account->id(); up so that other modules have a way to detect if it's a real logout or just a masquerading. See also #2124117: Unable to masquerade when logging in via SAML which tries to solve this in simplesamlphp_auth but currently requires an ugly hack to check the caller.
The template reset is IMHO not important, that would only matter if we would render anything in this context, but a) nothing has been rendered yet, so it hasn't been filled yet and b) we're not rendering anything after that.
Comment #4
berdirDiscussed with @andypost, he prefers the session-based approach, will also upload a patch to the simplesamlphp_auth issue mentioned above in a minute.
Comment #5
berdirComment #6
berdirNoticed this doesn't work yet for unmasquerade, might need a similar change for the switch back part.
Comment #7
andypost@Berdir is there a steps to test unmasquerade?
Comment #8
berdirHere is a patch that also switches the order for switchBack(), no additional changes are needed for simplesamlphp_auth.
Comment #9
andypostI bet it need
hasSession()on request before removal of flagComment #10
andypostFor myself to check with https://www.drupal.org/commitlog/commit/2/aaf13e2338771bef7a281fef7a72b3...
Comment #11
berdir@andypost: How is that related to that issue? I'm just moving the existing calls around. Even if there s a problem (which I doubt, because you can't get there without having a session/being logged in), it is not caused by this?
Comment #12
andypostI think this flag should be differently named, because this session is not really in action, so $this->session->set('masquerade_logout', TRUE) to allow properly react on login/logout
Comment #13
berdir@andypost: Not sure I follow? I'm only changing when the existing flag is set, we can't change the name of that? I could set a *second* additional flag, but not quite sure what the point of that would be? we could set it somewhere else than the session, but that would likely make the code in the simplesamlphp_auth more complex as it would need a module exists check or so.
Comment #14
joelpittet@andypost any further thoughts on this patch? I'm going to give it a whirl to see if it helps.
Comment #15
andypostSorry for taking long to review, goona commit because it only rearranges code
Comment #17
andypostThanks a lot for clean solution!