Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[11.x] Auth User Impersonation #51031
base: 11.x
Are you sure you want to change the base?
[11.x] Auth User Impersonation #51031
Changes from all commits
7bde396
b964824
5580480
565851b
a737274
44c7c66
388255c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regenerate()
doesn't remove any data from the session, it just rotates the ID, so this won't stop session attributes from bleeding across sessions.You can pass
regenerate($destroy = true)
to force it to wipe the session data when regenerating the session. I haven't tested it, but you should be able to destroy the session, start a new one, and write to it in the same request?There is still the risk that something in the current request bleeds through to the session after this though. Either due to Laravel persisting session data somewhere and rewriting it, or maybe through some middleware that manipulates the session after the controller builds a response.
Given I'm paranoid, I'd prefer a full bounce through the browser, i.e. kill the current session, redirect to a unique signed URL, build a new session, etc. It'd eliminate session bleed, but would also add implementation complexity. So maybe this is an acceptable risk that needs to be carefully outlined in the docs? Similar to how they'll need to advise devs to do proper authorisation checks before allowing a user to impersonate another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the session will be completely destroyed, I'd recommend we store the attributes in the session before impersonating into a key that can be reinstated (for example
_old.impersonated
) so that we don't loose the context of the original user when they leave the impersonation, otherwise important info may be lost.Although the way I have seen this implemented in Nova and other packages, the session is never destroyed nor regenerated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also rarely seen that impersonation regenerates session or data. It's not like impersonating grants you the users session data, it's still yours so there's nothing to actually leak. And also, if you can impersonate, you already see that users data so what are you actually leaking here in the session? Maybe all this needs is just to allow clearing current session data
impersonate($user, $clearData = true)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The risk is if there is sensitive information in the admin's session that gets stored when impersonating a user, as it'll be stored under the user's account - not the admin's account.
As an example, consider a form that accepts personal or sensitive information which stores draft responses in a session variable. The admin user fills in the form, submitting sensitive information which is stored in a session variable. The admin impersonates the user, visits the form, and the admin's sensitive information is stored in the user's account.
It's a pretty contrived example, but it's definitely possible if session data is persisted.
I can't see a reason why you'd want to persist session data when impersonating a user, so why make it optional? What data would you want to persist that would apply across both?
Also, it's best practice to regenerate sessions during login and logout to prevent session fixation, etc, so I think it makes sense to do the same for impersonation too. It's a change of security context, so session IDs and data shouldn't be persisted.
My recommendation is to force a logout to end impersonation, which removes the need for storing the old session. But if you want to end impersonation without logout, then storing the old session like this through regenerations makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see that being a problem, but I also think it's an implementation detail for the developer to decide whether they want to clear all or some of the data or not on impersonation.
We have a case where the user has an active_company_id and the id is not available via url - when impersonating we do not want to modify it for the user so the id is stored in the session for specificly targeted company as to not magically make the user start creating entries for different company he's not working with at the time.
We also usually have a previous url stored in the session when stopping impersonation to redirect back to as most time it's being tested with many different users at the same time and it's extremely annoying having to filter the list over and over again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any interest in
stopImpersonating()
orendImpersonation
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to choose stopImpersonating() instead of unpersonate() because I don’t see the vocabulary for unpersonate(). Hmm, I come with another option: stopImpersonate(). It saves more characters than stopImpersonating() although stopImpersonate() is same with stopImpersonating().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll wait for additional feedback on this as well. Chose this for its character length being the same, but maybe
startImpersonating()
stopImpersonating()
would work better like you guys mention 🙏There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about
impersonateAs()
andimpersonateOff
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There’s already precedence for
stopImpersonating
as that’s what the classic version of Spark used: https://github.com/laravel/spark-aurelius/blob/9db0edc5fe1d067305797a46e9e45f116890d476/src/Http/Controllers/Kiosk/ImpersonationController.php#L53It also makes sense as a method name since it clearly explains the action being performed.