Skip to content

feat(account): admin backend foundation — account disable/enable, admin audit log, admin list queries - #11055

Open
MichaelUray wants to merge 5 commits into
hcengineering:developfrom
MichaelUray:feat/admin-backend-foundation
Open

MichaelUray wants to merge 5 commits into
hcengineering:developfrom
MichaelUray:feat/admin-backend-foundation

Conversation

@MichaelUray

Copy link
Copy Markdown
Contributor

Context

Backend-only foundation for instance-admin tooling, as agreed in #10883 (#10883 (comment)). This PR has no UI. The admin panel UI from #10883 will be stacked on top of it in a follow-up.

It is a fresh port against current develop, not a rebase of #10883. It lives entirely in server/account and foundations/core/packages/account-client (plus a 3-line lockfile entry for the new workspace dependency).

What's in (one commit each)

  1. Data model + migration - account.disabled_at, account.token_version, account.last_activity_at, the admin_audit_log table, and indexes for the admin listing. One new migration, account_db_v28_account_lifecycle_admin_audit. AccountDB.adminAuditLog is added for PostgreSQL and MongoDB.
  2. Disabled-principal gate, token versioning, login checks
    • wrap() loads the account of every UUID token principal and answers Unauthorized if the account is disabled. This applies to session tokens, API tokens and user-scoped service tokens on every account RPC. Because the transactor/REST token check (getLoginInfoByToken) and WebSocket session setup (getLoginWithWorkspaceInfo) also go through wrap(), they are covered too.
    • Exempt: system, guest and read-only guest principals, non-UUID principals, and principals without an account row (for example the 2FA intermediate token).
    • The loaded row is passed to the method as meta.principalAccount, so the hot paths do not read the account a second time. wrap() never forwards a value supplied by the caller.
    • Session tokens carry token_version when it is greater than 0. getLoginInfoByToken, getLoginWithWorkspaceInfo and selectWorkspace reject session tokens issued before the last version bump. API tokens (extra.apiTokenId) and service tokens (extra.service) are not version-validated (a token re-minted from one, e.g. by getLoginInfoByToken or selectWorkspace, may carry token_version, but it is ignored), so they keep working after a bump; they are still rejected while the account is disabled. There is no version check in wrap() itself.
    • The token-less login paths refuse disabled accounts with account:status:AccountDisabled: password, OTP, 2FA, provider (OAuth/OIDC) and grant sign-in. Password login reports this only after the password has matched, so the disabled state is not disclosed to callers who do not know the password.
    • createApiToken additionally refuses disabled accounts and stale session tokens (defense in depth).
    • Successful logins and workspace selection record last_activity_at, at most one write per 5 minutes per account.
  3. disableAccount / enableAccount (admin-only RPCs and client methods)
    • Admin-only means a token with the admin claim and a current token version.
    • Disable sets disabled_at and bumps token_version. It refuses to disable the caller (CannotDisableSelf) and the last active instance admin from ADMIN_EMAILS (LastAdmin). The last-admin invariant is checked again after the write; if a concurrent disable left no active admin, the change is undone.
    • Enable clears disabled_at and bumps token_version, so session tokens issued before the disable stay invalid.
    • Disabling a disabled account, or enabling an active one, changes nothing (no version bump) and only writes an audit row with details.noop.
    • State change and audit row are written by AccountDB.applyAccountLifecycle. On PostgreSQL/CockroachDB both happen in one transaction, and a failed audit insert rolls the change back. On MongoDB they are written one after the other (documented as not atomic).
    • Refused attempts (self-disable, last admin) are audited as admin_action_denied. This is best-effort and throttled per admin, reason and target.
  4. Version check in token-minting operations - createAccessLink, sendInvite, resendInvite and changePassword reject stale session tokens. API and service tokens are not version-checked there (they are still refused while the owner is disabled); the API-token revocation and expiry check in wrap() is unchanged.
  5. listAccountsAdmin / listAuditAdmin (admin-only RPCs and client methods)
    • listAccountsAdmin filters by search, status, auth method, name/email, workspace membership, workspace count, last activity, orphan and instance admin. It supports sorting and offset pagination, all pushed down to SQL.
    • The SQL is safe by construction: parameters are bound, sort columns come from a whitelist with a uuid tie-breaker for stable pages, and LIKE wildcards are escaped (escapeLike).
    • listAuditAdmin returns entries newest first. Filters: target account, admin account, action and time range. It uses offset pagination and resolves admin and target names.
    • Request parameters are reduced to well-typed values before they reach the DB. Malformed workspace UUIDs and non-integer numbers (pagination, workspace-count and last-activity bounds) are rejected with BadRequest instead of failing in the database; limit is clamped to 500.

API tokens (#10624)

API tokens are not version-validated and are not revoked when an account is disabled. While the owner is disabled, the gate rejects them everywhere. That includes the transactor's revocation checker, which treats Unauthorized from getLoginInfoByToken as revoked. After enable they work again, including through getLoginInfoByToken, because the token-version check applies only to session tokens. Session tokens issued before the disable stay invalid after enable on the version-checked paths (getLoginInfoByToken, getLoginWithWorkspaceInfo, selectWorkspace, the four operations above and createApiToken) because of the version bump. The revocation and expiry check for API tokens in wrap() is unchanged.

Service tokens

Service tokens minted with the system account are exempt from the gate. Service tokens minted with a user's UUID are gated like sessions: telegram-bot, mail worker, events processor, the GitHub user token, and the AI bot's own account. So a disabled user's integrations stop working until the account is enabled again (see question 2). Like API tokens, they are not version-validated, so they work again after enable (including getLoginWithWorkspaceInfo and selectWorkspace).

Migration

  • account_db_v28_account_lifecycle_admin_audit contains only IF NOT EXISTS statements (ADD COLUMN, CREATE TABLE, CREATE INDEX). It has no backfill and no constraints.
  • It must also apply cleanly on deployments that already ran the earlier admin migrations from Admin Panel: Users + Workspaces + Audit log (V27 → V30) #10883 (account_db_v27_admin_user_management ... v30). An upgrade test reproduces exactly that state on PostgreSQL and CockroachDB: the legacy DDL is copied verbatim into a test fixture, applied, and then init() runs. The test then checks the schema and runtime behaviour, including listing legacy audit rows without a target account.
  • A second test runs init() twice on a fresh database and executes the DDL twice.
  • MongoDB only gets a new adminAuditLog collection. listAccountsAdmin/listAuditAdmin return account:status:NotSupportedOnBackend there, the same way the existing listAccounts is not implemented for MongoDB. disableAccount/enableAccount and the audit insert work on both backends.

Deliberately out (follow-ups)

  • The admin panel UI (Admin Panel: Users + Workspaces + Audit log (V27 → V30) #10883).
  • Workspace membership admin RPCs, createAccountAdmin, triggerPasswordReset, bulk operations, and workspace-operation audit rows. These need a nullable target_account and a batch id and would come as a second backend step.
  • CSV export and decodeFilterParam. I mentioned decodeFilterParam earlier, but I dropped it here because its only consumer is the CSV export, which belongs to the UI stage.
  • Realtime eviction of open WebSocket sessions on disable. Today an open session lives until reconnect; the reconnect itself and every account RPC are rejected.
  • Version checks on the remaining RPCs, audit retention, and API-token revocation on disable (see question 1).

Cost

One primary-key account lookup per authenticated account RPC, in wrap(). System-account service calls do not pay it. The row is reused through request meta, so getLoginInfoByToken, getLoginWithWorkspaceInfo, assertAdmin and createApiToken do not read it again. There is no cross-request cache because account pods scale out and there is no cheap invalidation. The transactor already caches getLoginInfoByToken for 60 s.

Compatibility

  • Tokens issued before this change have no version claim and stay valid until the account's first version bump.
  • The ADMIN_EMAILS admin gate is unchanged. The serveAccount signature is unchanged.
  • Meta gains an optional field. sendEmailConfirmation now takes db (exported helper; no callers outside server/account).
  • @hcengineering/account now depends on @hcengineering/account-client for the shared row type.
  • New status codes: account:status:AccountDisabled, CannotDisableSelf, LastAdmin, NotSupportedOnBackend.

Tests

  • server/account: 736 tests (530 before this PR), including the real-database suite against CockroachDB and PostgreSQL 18, as in CI. account-client: 42 tests (38 before).
  • New: disabled-principal gate matrix (session, API and service tokens, exempt principals, token-less RPCs, meta handling); login-path checks; token-version helpers, including that API and service tokens are rejected while disabled but not version-checked; disable/enable including no-op, last-admin race compensation and an end-to-end disable-to-enable round trip through the registered RPCs (getLoginInfoByToken, getLoginWithWorkspaceInfo, selectWorkspace and an ordinary wrapped RPC) for session, API and user-scoped service tokens; admin_action_denied throttling; the four token-minting operations, called directly and through wrap(); listAccountsAdmin BadRequest for malformed workspace UUIDs and fractional pagination at the RPC level.
  • Real-database tests on both flavors: upgrade fixture, double init, applyAccountLifecycle commit and rollback on a failed audit insert, every listAccountsAdmin sort field in both directions, filters, and listAuditAdmin.
  • Each of the five commits builds (rush validate) and passes the suites on its own.

Open questions

  1. API tokens on disable: they are rejected by the gate while the owner is disabled, but not revoked, so they work again after enable. Would you prefer to also revoke them on disable? That would be irreversible.
  2. User-scoped service tokens: tokens that integrations mint with a user's UUID (telegram-bot, mail, GitHub user token) are blocked while that user is disabled. Is that the intended behaviour, or should some services be exempt?
  3. Status codes: this PR adds a status block to the account plugin (AccountDisabled, CannotDisableSelf, LastAdmin, NotSupportedOnBackend) without client translations. Would you rather reuse platform.status.Forbidden/Unauthorized with details? If not, where should the translations live?
  4. Per-RPC account lookup in wrap(): is one primary-key lookup per authenticated RPC acceptable, or would you prefer a narrower projection without the password join?
  5. Open WebSocket sessions survive a disable until they reconnect. Is a follow-up for realtime eviction (queue-based force logout) wanted?
  6. MongoDB: lifecycle writes are sequential there, not transactional. Is that acceptable for the MongoDB backend?
  7. Stack: what shape do you prefer for the follow-ups? (a) workspace-membership admin RPCs, then (b) the UI from Admin Panel: Users + Workspaces + Audit log (V27 → V30) #10883, or both together?

…ration)

Add the data model for admin-driven account lifecycle management:

- account.disabled_at, account.token_version and account.last_activity_at
- admin_audit_log table with target/admin/time indexes
- supporting indexes for admin account listing queries

The schema is introduced by a single new migration,
account_db_v28_account_lifecycle_admin_audit. Every statement is
idempotent (IF NOT EXISTS), so it also applies cleanly on deployments
that already created these objects under the earlier admin migration
ids (account_db_v27_admin_user_management .. v30). A real-database
upgrade test reproduces that state on PostgreSQL and CockroachDB.

AccountDB gains an adminAuditLog collection (insert only for now) for
PostgreSQL and MongoDB; the account SELECT returns the new fields.

Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
…, disabled checks on login paths

Make a disabled account unusable everywhere, independent of how its
credentials were issued:

- wrap() loads the account of every UUID token principal and rejects the
  request with Unauthorized when the account is disabled. This covers
  session tokens, API tokens and user-scoped service tokens for every
  account RPC, and therefore also transactor/REST token verification
  (getLoginInfoByToken) and WebSocket session establishment
  (getLoginWithWorkspaceInfo). System, guest and read-only guest
  principals, non-UUID principals and principals without an account row
  are exempt. The loaded row is passed to the method as
  meta.principalAccount so hot paths do not read it twice; a
  caller-supplied value is never forwarded.
- Session tokens carry the account's token_version (when > 0);
  getLoginInfoByToken, getLoginWithWorkspaceInfo and selectWorkspace
  reject session tokens issued before the last version bump. API tokens
  (extra.apiTokenId) and service tokens (extra.service) are not
  version-validated (a re-minted one may carry the claim, which is
  ignored), so they keep working after a bump; they are still rejected
  while the account is disabled. There is no version check in wrap()
  itself.
- The token-less login paths (password, OTP, 2FA, provider and grant
  sign-in) refuse disabled accounts with account:status:AccountDisabled.
  Password login only reports it after the password matched.
- createApiToken additionally refuses disabled accounts and stale
  session tokens (defense in depth).
- Successful logins and workspace selection record last_activity_at,
  throttled to one write per five minutes.

API tokens stay unversioned and keep their own revocation check.

Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
…state+audit write

Add two admin-only account RPCs (token with the admin claim, current
token version):

- disableAccount sets disabled_at and bumps token_version. It refuses to
  disable the caller (account:status:CannotDisableSelf) and the last
  active instance admin from ADMIN_EMAILS (account:status:LastAdmin),
  re-checking the last-admin invariant after the write and undoing the
  disable if a concurrent change left no active admin. Disabling an
  already disabled account changes nothing.
- enableAccount clears disabled_at and bumps token_version, so session
  tokens issued before the disable stay invalid. Enabling an active
  account changes nothing.

Both record an admin_audit_log row (no-op calls are recorded with
details.noop). State change and audit row go through the new
AccountDB.applyAccountLifecycle, which runs them in a single transaction
on PostgreSQL/CockroachDB (a failed audit insert rolls the change back)
and sequentially on MongoDB. Refused attempts are audited best-effort as
admin_action_denied, throttled per admin, reason and target.

API tokens are deliberately not revoked: the disabled-principal gate
rejects them, like user-scoped service tokens, while the account is
disabled. Neither is version-validated, so both work again after
enable, while session tokens issued before the disable stay invalid.

The account client gains disableAccount and enableAccount.

Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
…dInvite, resendInvite and changePassword

These operations mint new access (guest links, invites) or change
credentials based on the caller's token. They now reject session tokens
issued before the account's last token version bump, using the account
row they already load.

API and service tokens are not version-validated here; they are
still refused while the owner is disabled. API token revocation and
expiry are still enforced by wrap().

Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
…L pushdown), escapeLike

Add two admin-only read RPCs:

- listAccountsAdmin lists accounts with search, status, auth method,
  name/email, workspace membership, workspace count, last activity,
  orphan and instance-admin filters, sorting and offset pagination.
  Filtering, sorting and paging are pushed down to SQL; user input is
  bound as parameters, sort columns come from a whitelist (with a uuid
  tie-breaker for stable pages) and LIKE wildcards are escaped
  (escapeLike).
- listAuditAdmin lists admin_audit_log entries newest first, filtered by
  target account, admin account, action and time range, with offset
  pagination and the admin/target names resolved.

Request parameters are reduced to well-typed values before they reach
the database; malformed workspace UUIDs and non-integer numbers
(pagination, workspace count and last-activity bounds) are rejected with
BadRequest. Both queries are implemented for PostgreSQL/CockroachDB;
on MongoDB they return account:status:NotSupportedOnBackend, as
listAccounts is likewise not implemented there.

The account client gains listAccountsAdmin and listAuditAdmin with
their DTOs; @hcengineering/account now depends on
@hcengineering/account-client for the shared row type.

Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
return await db.getWorkspaceUsersWithPermission(workspace, permission)
}

/* ======== ADMIN: ACCOUNT LIFECYCLE ======== */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that this file already contains too much. What do you think about moving the new admin operations to a separate file?

throw new PlatformError(new Status(Severity.ERROR, platform.status.BadRequest, {}))
}

const { account, extra } = decodeTokenVerbose(ctx, token)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like an old session can mint a current session after re-enable. createWorkspace accepts a session token without checking its version, then issues a token stamped with the account’s current version. A session issued before disable can therefore create a workspace after re-enable and obtain a usable token, bypassing the intended session invalidation. I suggest to check the incoming version before issuing a replacement token; audit the other token-returning RPCs for the same path.


// isAdmin filter
if (params.isAdmin === true) {
conds.push(`LOWER(s.primary_email) = ANY(${ph(adminEmails.map((e) => e.toLowerCase()))}::TEXT[])`)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the admin list can omit an actual admin. The admin filter examine only the minimum verified email. Login grants the admin claim using the email used to sign in. If an account has multiple verified emails and a configured admin email is not the minimum, it can act as an admin but disappears from isAdmin: true results and is labelled non-admin.

@ArtyomSavchenko

Copy link
Copy Markdown
Member

Here are review results by AI agent and the comments seem valid from my point of view.

Must fix

  1. Stale session tokens still work for most account calls after re-enable.
    wrap() already loads the account row on every request but only checks disabledAt. The token-version check runs only in getLoginInfoByToken, getLoginWithWorkspaceInfo, selectWorkspace, createAccessLink, sendInvite/resendInvite, changePassword and createApiToken.
    So after disable → enable, a pre-disable session token (likely the compromised one that prompted the disable) still works for createWorkspace, updateWorkspaceRole, deleteAccount, releaseSocialId, setMyProfile and others. The row is already loaded, so calling checkTokenVersionClaim in wrap() would close this at no extra cost and make the per-method checks unnecessary.

  2. Disabling doesn't end live sessions.
    The transactor checks the token only when a connection is made (sessionManager.addSession → getLoginWithWorkspaceInfo), so an open WebSocket keeps working until it reconnects. Services that verify JWTs locally (collaborator, datalake/files, …) never consult the account service, so they don't see the disable at all. That may be fine for a first PR, but the PR description should say so rather than claim the account is "unusable everywhere".

  3. Three different definitions of "admin".

    • Disable guard: isLastActiveAdmin counts any email social ID in ADMIN_EMAILS, verified or not, including ones with is_deleted set.
    • Account list: listAccountsAdmin sets isAdmin only when primary_email, the alphabetically smallest verified email (MIN(value)), is in the list. An admin whose admin address isn't their alphabetically first email shows as non-admin, and the isAdmin filter misses them.
    • Login: the admin claim depends on the email used to sign in.

    These should share one helper.

  4. The new error codes have no translations.
    account:status:AccountDisabled, CannotDisableSelf, LastAdmin and NotSupportedOnBackend aren't in any lang/*.json, so a disabled user trying to log in sees the raw status ID. (workbench-assets has an unrelated AccountDisabled string for the workspace-level flag.)

  5. A database hiccup now logs users out.
    In getLoginInfoByToken and getLoginWithWorkspaceInfo, verifyTokenVersion (which reads the database) runs inside the existing try, and that catch turns every error into Unauthorized. A brief database error becomes a forced logout. Every stale or disabled token is also reported to Analytics as an error with the full token in the log (ctx.error('Invalid token', { token })). Move the check out of that try, or let non-TokenError failures pass through as 500.

Should address

  • Account listing performance. The ws_counts and socials CTEs group the entire workspace_members and social_id tables on every call, twice (once for rows, once for the count). Filters and paging are applied after that, so the new indexes don't help. Fine for self-hosted, expensive at SaaS scale. Consider restricting to the page's account IDs first, or computing the counts in a lateral subquery after filtering.
  • Auth-method grouping is fork-specific. Only email: and oidc: keys count. Google- and GitHub-only users (SocialIdType.GOOGLE/GITHUB) are classed as none. The comment about "legacy rows with type values no longer in the enum" doesn't match upstream either.
  • A rolled-back disable still logs the target out. When the concurrent last-admin re-check undoes a disable, tokenVersion stays bumped (deliberately), so the target admin's sessions are invalidated anyway. Worth a comment, or skip the bump until after the re-check.
  • Disable targets aren't validated. The system account or a non-UUID account can be "disabled", which wrap() then ignores because those principals are exempt. Rejecting targets that fail isGatedPrincipal is cleaner.
  • Fork migration history in upstream code. The legacyAdminMigrations.ts fixture, the comment about deployments with account_db_v27..v30_admin_* IDs, and the AdminAuditLogListEntry.targetAccount: null handling all come from a fork. Fresh installs get target_account NOT NULL, while fork installs have it nullable plus a batch_id column and a CHECK constraint, so the schema differs between deployments. Maintainers should decide whether upstream should carry that.
  • Leftovers and small issues:
    • ListAccountsAdminQueryParams.lastActivityFilter has before/after/between variants and SQL branches that nothing can reach, since toListAccountsAdminQuery only maps never and range.
    • buildListAccountsAdminSql writes ${ns}.table directly; the other queries use getTableName(), which handles an empty ns.
    • listAuditAdmin uses asNumber for limit/offset, while listAccountsAdmin uses asInteger. It works because of Math.floor, but the two are inconsistent.
    • The client type makes pagination required, while the server treats it as optional.
    • The new CTE and the disable guard don't filter social_id.is_deleted.
  • Extra database reads. Every request with a user token now does one extra account read in wrap(), and every token issued without a preloaded row does another (generateTokenWithVersion). Probably fine, but it adds load on the transactor's connect path.
  • Scope. @hcengineering/account now depends on account-client just for a row type; that type could live in core or account types. Splitting the reads (commit 5) from the lifecycle/enforcement work (commits 1–4) would make review easier.

@MichaelUray

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. The points are valid; I'm reworking the PR accordingly, including the split into two PRs, and will push the updated version soon.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants