dhensby
commented
2 years ago This way of authenticating a member isn't really any different to how it works in core (see https://github.com/silverstripe/silverstripe-framework/blob/4/src/Security/MemberAuthenticator/SessionAuthenticationHandler.php).
a) I don't see a scenario where you want to masquerade as a user only when a particular get variable is in place or only on a subset of URLs. It would be very confusing for an admin who was masquerading to see the user context continually flipping. I think keeping users masquerading until they explicitly choose to log out is best
b) we could perform another permission check, but that would mean it would be slower and a) was already raising performance concerns. It's assumed if this session value is set the user can masquerade so I'm not entirely sure another check is needed
c) if a user has access to the session values then they don't need to worry about masquerading, they can just set the member ID if they want to elevate their permissions
This
https://github.com/dhensby/silverstripe-masquerade/blob/master/src/Control/MasqueradeMiddleware.php#L15-L20
Seems to be running on every request - do I see that right?
I wonder - if it would be better to ...
a. only run this on a specific URL, or in any way, run a super fast check - does this apply now. e.g. isset($_GET[....]) before running other logic. It may not make much difference, but right now you are dependent on the session being super fast and that is not really in your control.
b. check if the current user is allowed to "canMasquerade", otherwise, anyone who can hack the session variables has access to any user
c. in that light - would it be better if the user is identified with a secret code rather than just an ID