PR #614: Super-admin impersonation + /admin/users + /admin/audit-log#640
Conversation
…alogs) + #614c (broadcast) PR #614 scope as a single ticket was too broad (3 admin pages + impersonation engine + audit log + global users + 4 sub-features each non-trivial). Split into a sequenced set so each is reviewable: - #614 (in progress): impersonation engine + /admin/users + /admin/audit-log + 6 integration tests + red banner + rate limit + forbidden-action filter - #614a: /admin/system read-only health dashboard + audit-log CSV export - #614b: /admin/catalogs (global reference data CRUD) - #614c: /admin/broadcast (depends on PR #617 notifications fixes) TZ.md status marker updated to reflect the split.
…filter, /admin/users + audit-log filters
- ICurrentUserService gains IsImpersonating + ImpersonatedByUserId
- JwtTokenService.GenerateImpersonationToken issues a 60min token with claims
is_super_admin=false, impersonated_by_user_id, original_tenant_id, impersonation_reason
- IImpersonationService + ImpersonationService:
* StartAsync: validates target (not self, not super-admin, active), enforces 3/24h
rate limit by querying SuperAdminAuditLog, audits 'impersonate.start',
inserts in-app Notification (severity warning, body in Ukrainian with Kyiv
timestamp + reason), best-effort email
* EndAsync: requires impersonated_by_user_id claim, audits 'impersonate.end',
re-issues a fresh super-admin token for the original admin
* LogForbiddenAttemptAsync: writes 'impersonate.forbidden_attempt' audit row
- ForbiddenDuringImpersonationAttribute (filter): on 403 also writes the
forbidden-attempt audit row with the attempted route. Applied to:
AuthController.ChangePassword, ApiKeysController create + revoke,
UsersController.UpdateUserRole + ResetUserPassword
- ImpersonationController: POST /api/admin/impersonate (super-admin only),
POST /api/admin/impersonate/end (callable from impersonation token)
- AdminController: GET /api/admin/users (global search across tenants with
tenant name), GET /api/admin/audit-log gains action / adminUserId / fromUtc /
toUtc / tenantId filters
CREATE INDEX ix_superadminauditlogs_impersonation_ratelimit ON SuperAdminAuditLogs (AdminUserId, TargetId, OccurredAt DESC) WHERE Action = 'impersonate.start' Backs the 3/24h-per-(admin,target) rate-limit lookup in ImpersonationService.StartAsync. Partial filter keeps the index small as the audit log grows across all super-admin actions.
… with X-Test-ImpersonatedBy
- (1) non-super-admin caller -> 403
- (2) super-admin without MFA -> 403 + X-Mfa-Required
- (3) happy path -> 200 + 60min token + audit row + Notification row (Ukrainian
title 'Сесія імперсонації', warning severity, body contains the reason)
- (4) reason < 10 chars -> 400
- (5) rate limit: 4th impersonation in 24h on same target -> 429
(3 prior 'impersonate.start' rows pre-seeded directly into
SuperAdminAuditLogs to keep the test deterministic and fast)
- (6) forbidden action under impersonation token (POST /api/auth/change-password)
-> 403 AND writes a separate 'impersonate.forbidden_attempt' audit row
TestAuthHandler now also accepts X-Test-ImpersonatedBy for forbidden-action
coverage, matching the JWT contract (impersonation tokens always carry
is_super_admin=false).
All 441 tests pass (315 unit + 126 integration).
…min/audit-log - authStore: add impersonation slice (target + original snapshots, expires) - api/admin: startImpersonation / endImpersonation / listAdminUsers / listAdminAuditLog - ImpersonationBanner: red, non-closable, z-index 9999, full viewport, countdown, exit restores original token - /admin/users: global users table with reason modal (>=10 chars) + impersonate - /admin/audit-log: filtered audit log with expandable payload - i18n keys (uk + en) for users page, audit log, banner, modal
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd45e6addc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await _audit.LogAsync( | ||
| ActionEnd, | ||
| targetType: "User", | ||
| targetId: _currentUser.UserId, | ||
| before: null, |
There was a problem hiding this comment.
EndAsync calls _audit.LogAsync while the active principal is still the impersonated target user, so SuperAdminAuditService records AdminUserId as the target account (it derives this from ICurrentUserService.UserId). This misattributes impersonate.end events, breaks adminUserId filtering, and weakens audit accountability for who actually ended the session; the log path should pass/override the original adminId from ImpersonatedByUserId.
Useful? React with 👍 / 👎.
| setAuth( | ||
| impersonation.originalToken, | ||
| impersonation.originalEmail, | ||
| impersonation.originalRole, | ||
| impersonation.originalTenantId, |
There was a problem hiding this comment.
After POST /api/admin/impersonate/end, the code immediately restores impersonation.originalToken instead of using the fresh token returned by the backend. If the original token has expired during impersonation, the next API call fails with 401 even though /end succeeded, causing an avoidable forced re-login path; the exit flow should apply the response token as the source of truth.
Useful? React with 👍 / 👎.
| if (tenantId.HasValue) | ||
| { | ||
| var tid = tenantId.Value.ToString(); | ||
| items = items.Where(x => x.TargetId == tid).ToList(); | ||
| total = items.Count; |
There was a problem hiding this comment.
The tenantId predicate is applied only after Skip/Take, so each page is filtered in memory and total becomes the count of matches on that single page, not the full result set. This yields empty/partial pages and incorrect totals whenever tenant-matching rows fall outside the current page window; the tenant condition should be part of the database query before CountAsync and pagination.
Useful? React with 👍 / 👎.
… on ICurrentUserService Avoids breaking 5 unit-test stubs (FakeCurrentUserService + per-test TestCurrentUserService) without forcing them to update.
1 participant
Scope (locked per TZ ПУНКТ 14)
This is the core of PR #614. Sub-PRs #614a (system-wide actions), #614b (catalog editor), #614c (broadcast notifications) remain queued in ROADMAP Upcoming.
Backend
Migration
`20260425093014_AddImpersonationRateLimitIndex` — raw SQL partial composite index:
`CREATE INDEX IF NOT EXISTS ix_superadminauditlogs_impersonation_ratelimit ON SuperAdminAuditLogs (AdminUserId, TargetId, OccurredAt DESC) WHERE Action = 'impersonate.start';`
Tests (6 minimum, required)
`tests/AgroPlatform.IntegrationTests/SuperAdmin/ImpersonationIntegrationTests.cs`:
`TestAuthHandler` extended with `X-Test-ImpersonatedBy` header.
All 441 tests pass (315 unit + 126 integration).
Frontend
ROADMAP / TZ
ROADMAP physically split: PR #614 = core (this PR), and #614a / #614b / #614c queued separately under Upcoming.