fix: Server-level impersonation doesn't bypass 2FA when enabled #14340
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR enhances the server-level impersonation functionality to properly bypass 2FA when enabled and adds comprehensive audit trails for all impersonation events. The changes modify the admin panel's impersonation flow by:
-
Adding audit trail capabilities: The
AuditModuleis now imported into theAdminPanelModuleto enable tracking of impersonation events -
Capturing impersonator identity: The GraphQL resolver now uses the
@AuthUserdecorator to extract the admin user performing the impersonation and passes this ID to the service layer -
Implementing comprehensive security measures: The service method signature has been updated to accept an
impersonatorUserIdparameter, with UUID validation using Zod for all inputs, extensive logging with correlation IDs, and proper error handling -
Enabling 2FA bypass for impersonation tokens: The
LoginTokenService.generateLoginTokencall now includes impersonation-specific metadata ({ isImpersonation: true, impersonatorUserId }) as a fourth parameter, which allows the authentication system to properly bypass 2FA validation for legitimate admin impersonation attempts
The changes follow the existing service patterns while adding necessary security enhancements for administrative operations. The audit trail implementation ensures compliance requirements are met by tracking both impersonation attempts and successful token generation events.
Confidence score: 4/5
- This PR addresses a critical security requirement but introduces breaking changes that need coordination
- The comprehensive audit logging and 2FA bypass mechanism appear well-implemented following security best practices
- Pay close attention to the AdminPanelService method signature change and ensure all calling code is updated accordingly
3 files reviewed, 1 comment
packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts
Show resolved
Hide resolved
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:50655 This environment will automatically shut down when the PR is closed or after 5 hours. |
packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/auth/auth.resolver.ts
Outdated
Show resolved
Hide resolved
| workspaceId: string, | ||
| impersonatorUserId: string, | ||
| ) { | ||
| try { |
There was a problem hiding this comment.
This doesn't seem like the standard pattern for input validation in the codebase, usually it's validated at the graphql layer (ImpersonateInput). For impersonatorUserId why not but I don't think it should ever be corrupted unless it's a malicious attempt
| const loginToken = await this.loginTokenService.generateLoginToken( | ||
| user.email, | ||
| user.userWorkspaces[0].workspace.id, | ||
| undefined, |
There was a problem hiding this comment.
I don't really like this undefined option it feels like a codesmell. Maybe we should introduce Impersonation as an auth provider type?
There was a problem hiding this comment.
agreed, I felt the need to improve the undefined and was thinking for a better approach than this. Thanks for the advice, I'll add impersonation as an auth provider and import it here.
| `Impersonation token exchange attempt for ${email} by ${impersonatorUserId}`, | ||
| 'AuthResolver.getAuthTokensFromLoginToken', | ||
| ); | ||
| if (workspace.allowImpersonation !== true) { |
There was a problem hiding this comment.
We try to avoid nested if structures, and also avoid else {} statements, when possible (because they are hard to follow). Usually it's more elegant to use ifs with an early return / exception thrown
There was a problem hiding this comment.
Got it, I'll remove nested if and else and introduce a more flat approach.
…Admin Panel service and Auth resolver
|
Thanks @harshit078 for your contribution! |
…tyhq#14340) ## Description - This PR solves the a sub-issue from twentyhq/core-team-issues#1421 - impersonation tokens bypasses 2FA as intended - Added Audit trails to cover all impersonation events - Added Proper testing coverage --------- Co-authored-by: Félix Malfait <[email protected]>
Description