feat: Impersonate a user #25050
feat: Impersonate a user #25050
Conversation
862b38b to
acc8d5a
Compare
* feat: impersonate any user as Administrator (cherry picked from commit ecc9ff3) * fix: Flag impersonated sessions (cherry picked from commit c58ac80) * fix: log activity for impersonations (cherry picked from commit 0fd4d6b) * fix: track all impersonated changes (cherry picked from commit be47ee0) * fix: notify user that they were impersonated (cherry picked from commit 24499d9) --------- Co-authored-by: Ankush Menat <ankush@frappe.io>
# [15.16.0](v15.15.0...v15.16.0) (2024-02-28) ### Bug Fixes * **a11y:** Improve desk accessibility (backport [#23319](#23319)) ([#25023](#25023)) ([48665e6](48665e6)) * add context for row number label ([235f921](235f921)) * add show_dashboard field on custom fields ([#24984](#24984)) ([#24986](#24986)) ([34bca99](34bca99)) * added important to bold class. ([718973e](718973e)) * allow negative numbers in grid search ([#24989](#24989)) ([#24991](#24991)) ([e778fe9](e778fe9)) * Avoid add "null" to cc in communication in timeline (backport [#25092](#25092)) ([#25096](#25096)) ([06c7a87](06c7a87)) * Cast to string to handle int PK ([#24988](#24988)) ([#24993](#24993)) ([de3042b](de3042b)) * Check perm for library file before cloning ([#25117](#25117)) ([#25120](#25120)) ([173ff30](173ff30)) * **ControlTime:** Don't set datepicker's date if field is read only ([#25038](#25038)) ([#25076](#25076)) ([3e33ae3](3e33ae3)) * Correct type hint ([#24990](#24990)) ([#24995](#24995)) ([3f956c1](3f956c1)) * disable internal columns like _comments from report column selec… ([#24998](#24998)) ([#25000](#25000)) ([76a32f2](76a32f2)) * Don't init site if already init-ed during truncate ([#25033](#25033)) ([#25036](#25036)) ([5ddbfb2](5ddbfb2)) * **File Uploader:** call 'upload_files' without passing event object ([#25034](#25034)) ([#25087](#25087)) ([ce5abfc](ce5abfc)) * force `[]` as default for child tables ([#24000](#24000)) ([fea1623](fea1623)) * **formatters:** Translate Select value in format_value ([#24951](#24951)) ([#25077](#25077)) ([ff9e199](ff9e199)) * german translation of No. ([4e491b2](4e491b2)) * grid row default values not using model (backport [#23701](#23701)) ([#24896](#24896)) ([ad05acb](ad05acb)) * **grid:** Add type attribute to buttons ([#25021](#25021)) ([#25022](#25022)) ([d440fef](d440fef)) * Guess currency from report row if available ([#25009](#25009)) ([#25010](#25010)) ([dfcee61](dfcee61)) * handle total rows in number card ([#25011](#25011)) ([#25012](#25012)) ([4882bfc](4882bfc)) * invalid lru_cache usage ([#25046](#25046)) ([#25047](#25047)) ([e275851](e275851)) * **module_map:** only include apps installed on the site - not everything on the bench (backport [#24688](#24688)) ([#25122](#25122)) ([79e7c57](79e7c57)) * Private images in PDFs from background jobs ([#24980](#24980)) ([#25015](#25015)) ([3211ccf](3211ccf)) * replaced created by with owner in base_document ([#25059](#25059)) ([#25061](#25061)) ([bc5d09d](bc5d09d)) * restrict method for security critical endpoints ([#25105](#25105)) ([#25108](#25108)) ([44f4858](44f4858)) * ruff fixes ([c557ec6](c557ec6)) * show tooltip on edit table row (backport [#25040](#25040)) ([#25069](#25069)) ([2afbed7](2afbed7)) * spelling of "recording" in button (backport [#25025](#25025)) ([#25026](#25026)) ([6bf4f1c](6bf4f1c)) * support running QB union queries (backport [#24757](#24757)) ([#25016](#25016)) ([c092efa](c092e 8000 fa)) * translate doctype in user-facing error message ([#25045](#25045)) ([3cd1eb2](3cd1eb2)) * validate fetch from ([#25116](#25116)) ([#25118](#25118)) ([624985d](624985d)) * wrap read_only functions correctly ([#25018](#25018)) ([#25020](#25020)) ([88a09be](88a09be)) ### Features * add push notification ([93ecd40](93ecd40)) * hook for print format template loader ([#25037](#25037)) ([#25074](#25074)) ([201e30f](201e30f)) * Impersonate a user (backport [#25050](#25050)) ([#25051](#25051)) ([813fb59](813fb59)) * socketio using authorization headers (backport [#24858](#24858)) ([#24966](#24966)) ([2045340](2045340)) * support array request type ([#25109](#25109)) ([#25112](#25112)) ([957d3eb](957d3eb)) ### Performance Improvements * avoid unnecessary json parsing ([#25065](#25065)) ([#25073](#25073)) ([1b814b3](1b814b3)) * don't process checks if there are none ([#25063](#25063)) ([#25071](#25071)) ([c3b0931](c3b0931)) * remove specific elements instead of re-rendering entire list ([#25078](#25078)) ([#25089](#25089)) ([37c9d3f](37c9d3f))
|
This is a neat feature. However, i think there should be a way for the Administator to end the impersonation without having to logout. Maybe add a button to say "End Impersonation"? to terminate the session? |
|
@flexcomng that's kinda dangerous from security POV. You mean reverting back to admin session right? |
|
@ankush Yes what i mean is reverting back to admin session without having to logout and re-authenticate. I don't see how that can be dangerous from a security perspective because if an administrator is allowed to impersonate a user without providing authentication, i believe it should also be possible to end that impersonation without having to log out and re-authenticate to get back into the application as administrator. What this should essentially entail is reversing all the permissions/access granted when the impersonation was triggered but leave the session logs for audit and history purposes. This will improve the user experience of the feature which is already an amazing addition. |
|
From security POV if current session data can be modified by a user then user can declare their session as impersonated somehow and access Admin account. Not sure how it would be possible right now... any idea @sagarvora 👀 The privesc risk seems high enough to me to sacrifice UX TBH. |
|
@ankush I am trying to understand what privacy concerns are at play here if an administrator is already allowed access to a user's account and can carry out actions on behalf of the user without authenticating as that user. More so, the feature already has an effective tracking process for all actions carried out on behalf of the user by the administrator. The ask here is simply to remove the access granted by the Impersonate User action without having to logout. It is not to remove the trail of activity by the administrator while operating as the user or hide any action carried out by the administrator. Although i have not delved into 67E6 the underlying code of the feature, i am assuming there is no timestamp tracking for session start and session end and based on my initial use of the feature. Also, unless the user is actually online and actually active when they are impersonated, they will not get notified of the impersonation via email which i believe should be an essential part of the feature. This part is more of a security risk than simply allowing the administrator to exit the impersonation mode without having to logout and there are so many actions an administrator can already carry out without requiring user permissions. Such an email notification should be triggered at the backend and an administrator should not be able to intercept it in the email queue. If the main benefit of this feature is to be able to troubleshoot from the viewpoint and perspective of the user, and safeguards have already been built in to ensure any action carried out by administrator is tracked and logged while impersonating, the logical next step is to allow the administrator to be able to get out of that session as simply as they got in and a start time and end time logged to show when they entered and exited the impersonation mode. |
|
@flexcomng I'm not worried about administrator being able to do something. I'm thinking of reverse, if normal user can somehow flag their session as impersonated then they'll able to "end impersonation" and become admin. |
@ankush Not if the End Impersonation action is available to the user. As i said earlier, i haven't fully explored the feature but If a user is able to end the impersonation session from their end if they believe the impersonation is unauthorised based on current implementation, and I would assume you believe this scenario could occur then safeguards should as well be built into it. If a user terminates an administrator's impersonation then i believe it is in this scenario that the administrator session should be thrown out by logging them out as that could be a potential access breach. Allowing a user to then assume the administrator role if they trigger end impersonation will in fact then be the security issue you are raising. If a user is allowed to end the impersonation session then the session should be terminated and the user's default permissions restored. There are additional safeguards i could suggest to make the feature robust security-wise as this feature could be indispensible in the near term. But focus should be on all aspects of the feature, from UI/UX to security and privacy. Because this will essentially be Frappe's "Remote Desktop". |
|
This feature is scary as is. Reverting it ultimately makes it scarier IMHO. |
I don't see how an administrator being able to provide first-party (direct) support to a user scary. A system administrator can already take over a computer remotely. A Frappe instance administrator can already literally do anything and everything without restrictions so i don't see how being able to troubleshoot a system with the same limitations the user without having ask for the user's credentials/changing their passwords or physically accessing the user's workstation. |
|
ERPNext is a solid product because we are absolutely sure each record has a timeline at the bottom and we are absolutely sure who changed what. This feature is just creating doubts. |
|
+1 to revert to admin session 👍🏻 |
@TurkerTunali this was taken care of by mentioning who actually did the change. See above PR description. |
This is often requested feature from support teams who need to login as another user to check some problem. Building a full fledged impersonation abstraction is probably not feasible, this PR just allows admin to login as any user.
Measures taken to avoid potential abuse:
TODO: