Skip to content

fix: deleting a user leaves its identity mappings (M41) - #77

Open
alghanim wants to merge 1 commit into
bodaay:masterfrom
alghanim:fix/delete-user-mappings
Open

alghanim wants to merge 1 commit into
bodaay:masterfrom
alghanim:fix/delete-user-mappings

Conversation

@alghanim

Copy link
Copy Markdown
Collaborator

Problem

DeleteUser removed only the user row, in both stores. The user's identity mappings, global roles and direct
permissions stayed, so local:<username> kept resolving to a GUID with no user:

  • POST /api/admin/bootstrap answered created: false for a deleted username. It set roles on a GUID with no user
    and never created the account again. We hit this re-bootstrapping a deployment after deleting a test user.
  • LDAP import reported a deleted user as already imported (status: exists).
  • App self-registration refused the username with 409 when an app-local user had been deleted through
    DELETE /api/admin/users/{guid}. M14 fixed only DELETE /api/app/users/{guid} and noted the admin path as a
    follow-up. The admin path also kept the app's role assignment, which is keyed by username, so a user created again
    under that name inherited the old roles.

Found while fixing it: DELETE /api/app/users/{guid} never revoked the deleted user's tokens or SSO sessions. The
admin delete did. A token issued before the delete kept working until it expired.

Fix

  • DeleteUser deletes, in the same transaction as the user row, the identity mappings the user still owns, its
    global roles and its direct permissions.

    • Bolt: deletes only forward entries the user still owns. The owner-scoped loop from DeleteApp (H8 / H14)
      is now a shared helper, deleteOwnedMappings; DeleteApp behaves exactly as before.
    • Postgres: deletes by user_guid / guid.
  • Both delete handlers share two helpers:

    • deleteLocalUser clears an app-local user's role assignment in its app. It reads everything before deleting, and
      returns a 500 with a clear message if the assignment cannot be saved, instead of ignoring the failure.
    • purgeDeletedUserState revokes tokens and sessions. It moved out of handleDeleteUser, so the app path now
      revokes them too.

    Both handlers take localUserMu, so an app-local create of the same username cannot run between the delete and
    the cleanup.

  • On open, both stores log every mapping that already points at a deleted user (provider=… external_id=…).
    They leave these in place by default and remove them only with SA_REPAIR_DANGLING_MAPPINGS=1.

    • Postgres then deletes exactly the logged rows.
    • For an app-local mapping, the repair also clears the app's role assignment for that username, unless a live user
      can still match the name (assignment keys are shared with directory users).
    • SA_SKIP_MAPPING_REPAIR=1 overrides the opt-in.
    • TestPostgresConnection skips the check.
  • Docs:

    • SECURITY-AUDIT.md: Audit Pass 6 / M41 entry and Status Summary row.
    • docs/API.md: what the two delete routes remove.
    • docs/CONFIGURATION.md: the two startup switches.

Behaviour change

Before this change, deleting an LDAP/Kerberos user also kept them out by accident. The leftover ldap: mapping
resolved to the deleted GUID and the login failed. Now the mapping goes with the user, so the user's next login
provisions a new account. That account picks up any app role assignments still kept under their username or
sAMAccountName, and their group assignments. To keep a directory user out, disable the account in SimpleAuth
(PUT /api/admin/users/{guid}/disabled) or in the directory (H17).

That is also why the startup check only reports by default. Removing old dangling ldap/kerberos mappings would
let users deleted before this change sign in again. Operators can review the log, then:

  • remove single mappings with DELETE /api/admin/users/{guid}/mappings/{provider}/{external_id} (this does not
    clear app role assignments), or
  • remove them all with SA_REPAIR_DANGLING_MAPPINGS=1.

No API shape changes; the SDKs are unaffected. A restored Bolt backup is checked on the next start.

Tests

  • internal/store/delete_user_test.go (Bolt) covers:
    • the cascade frees the username;
    • a mapping the forward bucket gives to another user survives the cascade;
    • the startup check is report-only by default, removes on opt-in, and the skip switch overrides the opt-in;
    • the repair clears a stale app-local assignment but keeps one a live user still matches.
  • internal/store/postgres_delete_user_test.go covers the same for Postgres, plus TestPostgresConnection not
    writing. These run when SA_TEST_POSTGRES_DSN is set, are skipped otherwise, and clean up their rows.
  • internal/handler/delete_user_m41_test.go covers:
    • after an admin delete, bootstrap creates the user again and the user can sign in;
    • an admin delete of an app-local user clears its role assignment;
    • an app delete revokes the user's access and refresh tokens.

These tests fail on master:

  • the cascade, repair-on-opt-in and bootstrap tests;
  • the admin-delete assignment test ("the new agent2 inherited the deleted user's roles [Supervisor]");
  • the token test: without the revocation, the old access token still validates and userinfo answers 404 instead
    of 401.

These describe behaviour master already has and guard against regressions:

  • the owner-scoped delete;
  • the report-only default;
  • the skip-switch test.

go build ./..., go vet ./internal/..., gofmt and go test ./... pass. go test ./... ran with the Postgres
tests enabled, against Postgres 16.

🤖 Generated with Claude Code

DeleteUser removed only the user row. The user's identity mappings kept
resolving to a GUID with no user, so bootstrap and LDAP import treated a
deleted username as existing, and app self-registration answered 409.

- DeleteUser now removes the mappings the user still owns, its global
  roles and its direct permissions, in the same transaction (Bolt and
  Postgres). DeleteApp's owner-scoped loop is now the shared helper
  deleteOwnedMappings.
- Both delete handlers clear an app-local user's role assignment and
  revoke tokens and sessions. DELETE /api/app/users/{guid} did not
  revoke them before.
- On open, both stores log mappings that point at deleted users. They
  remove them only with SA_REPAIR_DANGLING_MAPPINGS=1, because removing
  an old ldap/kerberos mapping lets that deleted user sign in again.

Behaviour change: a deleted directory user can now sign in again and is
provisioned a new account. Disable the account to keep them out.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

1 participant