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

SECURITY-AUDIT.md

@@ -117,6 +117,7 @@ source of truth for what is currently open vs. fixed.
 | M40 | v1 `/api/auth/refresh` copies the stored `aud` forward forever → an admin audience correction never reaches existing sessions | MEDIUM | FIXED | 2026-10-05 (Pass 5, branch `fix/code-review-findings`) |
 | L23 | SSO cookie path moved from `/` to `BasePath` without expiring the old `Path=/` cookie → legacy cookie survives logout and silently signs the user back in | LOW | FIXED | 2026-10-05 (Pass 5, branch `fix/code-review-findings`) |
 | L24 | `handleCreateLocalUser` ignores `SetIdentityMapping` / `GetAppAuthz` errors → nil-deref panic, half-provisioned user | LOW | FIXED | 2026-10-05 (Pass 5, branch `fix/code-review-findings`) |
+| M41 | Deleting a user leaves its identity mappings, roles and permissions → username cannot be provisioned again; admin delete of an app-local user keeps its role assignment; app delete does not revoke tokens | MEDIUM | FIXED | 2026-10-14 (Pass 6, branch `fix/delete-user-mappings`) |
 | L6 | `ValidateToken` did not require `exp` (missing-`exp` token validated) | LOW | FIXED | 2026-06-01 (Pass 3) |
 | L7 | PKCE accepted `plain`/empty downgrade though discovery advertises only S256 | LOW | FIXED | 2026-06-01 (Pass 3) |
 | L8 | Impersonation issued a token for a disabled / access-revoked target | LOW | FIXED | 2026-06-01 (Pass 3) |
@@ -1399,3 +1400,88 @@ that name.
 after the user was already created. The `SetIdentityMapping` error was also
 dropped. **Fix:** load authz before creating anything; on a mapping or authz-save
 failure, roll back the mapping and user and return 500.
+
+## Audit Pass 6 — 2026-10-14 — Claude Opus 5.5 (`claude-opus-5-5`) + operator report
+
+Scope: one issue an operator hit while re-bootstrapping a deployment ("bootstrap
+says the user exists, but the user is gone"), and the related paths found while
+fixing it. Fix on branch `fix/delete-user-mappings`.
+
+### M41 — Deleting a user leaves its identity mappings, roles and permissions — MEDIUM — FIXED 2026-10-14
+
+This closes the follow-up noted under M14: the admin `handleDeleteUser` path had
+the same dangling-mapping pattern.
+
+`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` resolved the mapping and answered `created: false`
+  for the deleted user. It set roles on a GUID with no user and never created the
+  account again.
+- LDAP import (`handleImportLDAPUsers`) reported the deleted user as already
+  imported (`status: exists`).
+- App self-registration refused the username with 409 for an app-local user
+  deleted through `DELETE /api/admin/users/{guid}`; M14 fixed only
+  `DELETE /api/app/users/{guid}`. The admin path also left the app's role
+  assignment, which is keyed by username, so a user created again under that
+  name would have inherited the old roles.
+- An LDAP or Kerberos login of a deleted directory user resolved the mapping to
+  the deleted GUID and failed. Deleting a directory user therefore blocked them
+  from signing in again, as an undocumented side effect.
+
+Found while fixing it: `DELETE /api/app/users/{guid}` never revoked the deleted
+user's access tokens, refresh tokens or SSO sessions. The admin delete did. A
+token issued before an app-local user was deleted stayed valid until it expired.
+
+No access is gained through the leftover mappings themselves: GUIDs are random
+and never reused.
+
+**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, using the helper
+  `deleteOwnedMappings` it now shares with the `DeleteApp` cascade (H8 / H14).
+  Postgres deletes by `user_guid`.
+- Both delete handlers go through the same code:
+  - `deleteLocalUser` clears the app's role assignment for an app-local user. It
+    reads everything before deleting, and reports a failed assignment save
+    instead of ignoring it.
+  - `purgeDeletedUserState` revokes tokens and sessions on both paths.
+  - Both 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 points at a deleted user and leave
+  it in place. They remove these only when the operator sets
+  `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.
+  `SA_SKIP_MAPPING_REPAIR=1` overrides the opt-in. `TestPostgresConnection`
+  skips the check. A restored Bolt backup is checked on the next start.
+
+**Operator notes (behaviour change):**
+- Deleting a directory (LDAP/Kerberos) user no longer blocks them. Their next
+  login provisions a new account with a new GUID. That account immediately gets
+  any per-app role assignment keyed by their username or sAMAccountName, and any
+  group assignment. To keep a directory user out, disable the account in
+  SimpleAuth or in the directory (H17); do not rely on delete.
+- The startup check is report-only because removing old dangling `ldap` or
+  `kerberos` mappings lets those deleted users sign in again. Review the logged
+  mappings first. Remove single ones with
+  `DELETE /api/admin/users/{guid}/mappings/{provider}/{external_id}`, which
+  does not clear app role assignments. Or remove all of them with
+  `SA_REPAIR_DANGLING_MAPPINGS=1`.
+
+**Tests:**
+- `internal/store/delete_user_test.go` (Bolt) covers:
+  - the cascade;
+  - the owner-scoped delete;
+  - report-only by default, removal on opt-in, and the skip switch;
+  - clearing the app assignment while keeping a name a live user matches.
+- `internal/store/postgres_delete_user_test.go` covers the same for Postgres,
+  plus `TestPostgresConnection` never writing. These run when
+  `SA_TEST_POSTGRES_DSN` is set 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 tokens.

docs/API.md

@@ -1100,6 +1100,12 @@ curl -k -X PUT https://auth.example.com/sauth/api/admin/users/550e8400-e29b-41d4
 
 Delete a user.
 
+The delete also removes the user's identity mappings, global roles, direct permissions, tokens and SSO sessions,
+and, for an app-local user, its role assignment in the app. The username is free to be provisioned again. A deleted
+directory (LDAP/Kerberos) user who signs in again gets a new account, which picks up any app role assignments still
+kept under their username. To keep someone out, disable the account instead
+(`PUT /api/admin/users/{guid}/disabled`).
+
 ```bash
 curl -k -X DELETE \
   -H "Authorization: Bearer ADMIN_KEY" \
@@ -2844,7 +2850,7 @@ credential (Basic or management token):
 | `POST` | `/api/app/users` | Create an app-local user `{username, password, display_name?, email?, roles?}`. |
 | `GET` | `/api/app/users` | List this app's local users. |
 | `PUT` | `/api/app/users/{guid}/password` | Reset a local user's password. |
-| `DELETE` | `/api/app/users/{guid}` | Delete a local user (must be owned by this app). |
+| `DELETE` | `/api/app/users/{guid}` | Delete a local user (must be owned by this app). Also clears its role assignment in the app and revokes its tokens and SSO sessions. |
 
 ```bash
 curl -u portal:$APP_SECRET -X POST …/sauth/api/app/users \

docs/CONFIGURATION.md

@@ -568,3 +568,13 @@ Combined, this means an admin can fully sign a user out of every app in under a
 From v2.3.0, SimpleAuth checks in Active Directory whether a directory user's account is still active (not disabled, not expired, not deleted) at every login, token refresh, and SSO-cookie reuse. The runtime settings `directory_outage_policy` (`grace` default / `block` / `allow`) and `directory_outage_grace_hours` (default `10`) decide what happens when AD cannot be reached.
 
 Full reference, including exact behavior, API examples, and troubleshooting: [ACTIVE-DIRECTORY.md → Disabled, Expired, and Deleted AD Accounts](ACTIVE-DIRECTORY.md#disabled-expired-and-deleted-ad-accounts).
+
+## Startup Data Checks
+
+On every start, SimpleAuth checks its identity mappings. Two environment switches control what it may change. They
+are not config-file keys.
+
+| Env Variable | Default | Description |
+|---|---|---|
+| `SA_REPAIR_DANGLING_MAPPINGS` | (unset) | Set to `1` to remove identity mappings that point at a deleted user. Versions before the M41 fix left them behind when a user was deleted. For an app-local mapping, this also clears the app's role assignment for that username, unless a live user still matches the name. When unset, each such mapping is only logged. Removing an `ldap` or `kerberos` mapping lets that deleted user sign in again with a new account, so review the log first. To remove mappings one at a time, use `DELETE /api/admin/users/{guid}/mappings/{provider}/{external_id}`. |
+| `SA_SKIP_MAPPING_REPAIR` | (unset) | Set to `1` to make both startup mapping repairs (H14 and M41) report without writing. Overrides `SA_REPAIR_DANGLING_MAPPINGS`. |

internal/handler/admin.go

@@ -1,6 +1,7 @@
 package handler
 
 import (
+	"errors"
 	"fmt"
 	"log"
 	"net/http"
@@ -186,32 +187,51 @@ func (h *Handler) handleDeleteUser(w http.ResponseWriter, r *http.Request) {
 	guid := pathParam(r, "guid")
 	ip := getClientIP(r)
 	log.Printf("[admin] User deleted guid=%s ip=%s", guid, ip)
-	if err := h.store.DeleteUser(guid); err != nil {
+	// An app-local user also has a role assignment in its app, keyed by
+	// username; deleteLocalUser clears it (M41).
+	h.localUserMu.Lock()
+	var err error
+	if u, gerr := h.store.GetUser(guid); gerr == nil && u.OwnerAppID != "" {
+		err = h.deleteLocalUser(u.OwnerAppID, guid)
+	} else {
+		err = h.store.DeleteUser(guid)
+	}
+	h.localUserMu.Unlock()
+	if err != nil && !errors.Is(err, errLocalAssignmentCleanup) {
 		jsonError(w, "failed to delete user", http.StatusInternalServerError)
 		return
 	}
-	// Invalidate everything tied to the deleted user so a still-live token or SSO
-	// session cannot keep acting as them: blacklist outstanding access tokens,
-	// drop refresh-token families, and clear shared SSO sessions. Best-effort —
-	// the user row is already gone (errors logged, not fatal).
+	h.purgeDeletedUserState(guid)
+	h.audit("user_deleted", "admin", ip, map[string]interface{}{"target_guid": guid})
+	if err != nil {
+		jsonError(w, err.Error(), http.StatusInternalServerError)
+		return
+	}
+	jsonResp(w, map[string]string{"status": "deleted"}, http.StatusOK)
+}
+
+// purgeDeletedUserState invalidates everything tied to a deleted user, so a
+// still-live token or SSO session cannot keep acting as them: it blacklists
+// outstanding access tokens, drops refresh-token families, clears shared SSO
+// sessions, UI preferences and the directory-check timestamp. Best effort: the
+// user row is already gone, so errors are logged, not returned.
+func (h *Handler) purgeDeletedUserState(guid string) {
 	if err := h.store.RevokeAllUserAccessTokens(guid, time.Now().Add(h.cfg.AccessTTL)); err != nil {
-		log.Printf("[admin] delete-user: revoke access tokens failed guid=%s err=%v", guid, err)
+		log.Printf("[delete-user] revoke access tokens failed guid=%s err=%v", guid, err)
 	}
 	if err := h.store.RevokeUserTokens(guid); err != nil {
-		log.Printf("[admin] delete-user: revoke refresh tokens failed guid=%s err=%v", guid, err)
+		log.Printf("[delete-user] revoke refresh tokens failed guid=%s err=%v", guid, err)
 	}
 	if err := h.store.DeleteUserSessions(guid); err != nil {
-		log.Printf("[admin] delete-user: clear SSO sessions failed guid=%s err=%v", guid, err)
+		log.Printf("[delete-user] clear SSO sessions failed guid=%s err=%v", guid, err)
 	}
 	if err := h.store.DeleteConfigValue(userPrefsKeyPrefix + guid); err != nil {
-		log.Printf("[admin] delete-user: clear UI preferences failed guid=%s err=%v", guid, err)
+		log.Printf("[delete-user] clear UI preferences failed guid=%s err=%v", guid, err)
 	}
 	h.dirChecked.Delete(guid)
 	if err := h.store.DeleteConfigValue(dirCheckKeyPrefix + guid); err != nil {
-		log.Printf("[admin] delete-user: clear directory-check timestamp failed guid=%s err=%v", guid, err)
+		log.Printf("[delete-user] clear directory-check timestamp failed guid=%s err=%v", guid, err)
 	}
-	h.audit("user_deleted", "admin", ip, map[string]interface{}{"target_guid": guid})
-	jsonResp(w, map[string]string{"status": "deleted"}, http.StatusOK)
 }
 
 func (h *Handler) handleSetPassword(w http.ResponseWriter, r *http.Request) {

internal/handler/app_selfservice.go

@@ -1,6 +1,8 @@
 package handler
 
 import (
+	"errors"
+	"log"
 	"net/http"
 	"time"
 
@@ -352,33 +354,70 @@ func (h *Handler) handleListLocalUsers(w http.ResponseWriter, r *http.Request) {
 func (h *Handler) handleDeleteLocalUser(w http.ResponseWriter, r *http.Request) {
 	appID := appIDFromContext(r)
 	guid := pathParam(r, "guid")
+	// Same lock as create, so a create of this username cannot run between the
+	// delete and the assignment cleanup.
+	h.localUserMu.Lock()
+	defer h.localUserMu.Unlock()
 	u, err := h.store.GetUser(guid)
 	if err != nil || u.OwnerAppID != appID {
 		jsonError(w, "user not found", http.StatusNotFound)
 		return
 	}
-	mapKey := "applocal:" + appID
-	mappings, _ := h.store.GetMappingsForUser(guid)
-	if err := h.store.DeleteUser(guid); err != nil {
+	err = h.deleteLocalUser(appID, guid)
+	if err != nil && !errors.Is(err, errLocalAssignmentCleanup) {
 		jsonError(w, "failed to delete user", http.StatusInternalServerError)
 		return
 	}
-	// Drop the user's identity mapping(s) so the username frees up for
-	// re-provisioning (otherwise the dangling mapping made create return 409
-	// forever) and the stale per-app role assignment doesn't outlive the user (M14).
+	h.purgeDeletedUserState(guid)
+	h.audit("app_local_user_deleted", h.appActor(r), getClientIP(r), map[string]interface{}{"app_id": appID, "guid": guid, "actor_kind": h.appActorKind(r)})
+	if err != nil {
+		jsonError(w, err.Error(), http.StatusInternalServerError)
+		return
+	}
+	jsonResp(w, map[string]string{"status": "deleted"}, http.StatusOK)
+}
+
+// errLocalAssignmentCleanup means the user was deleted but the app's role
+// assignment for its username could not be cleared.
+var errLocalAssignmentCleanup = errors.New("user deleted, but clearing its role assignment in the app failed")
+
+// deleteLocalUser deletes an app-local user of appID and the app's role
+// assignment kept under its username. DeleteUser removes the user's identity
+// mappings, which frees the username (M14, M41). The assignment is keyed by
+// username, not GUID, so it must go too, or a user created again under that
+// name would inherit the old roles. Everything is read before anything is
+// deleted, so a read failure leaves the user in place. Callers hold localUserMu
+// and call purgeDeletedUserState afterwards, also on errLocalAssignmentCleanup.
+func (h *Handler) deleteLocalUser(appID, guid string) error {
+	mappings, err := h.store.GetMappingsForUser(guid)
+	if err != nil {
+		return err
+	}
+	authz, err := h.store.GetAppAuthz(appID)
+	if err != nil {
+		return err
+	}
+	if err := h.store.DeleteUser(guid); err != nil {
+		return err
+	}
+	if authz == nil || authz.UserAssignments == nil {
+		return nil
+	}
+	mapKey := "applocal:" + appID
+	changed := false
 	for _, m := range mappings {
-		_ = h.store.DeleteIdentityMapping(m.Provider, m.ExternalID)
-		if m.Provider == mapKey {
-			if authz, err := h.store.GetAppAuthz(appID); err == nil && authz.UserAssignments != nil {
-				if _, ok := authz.UserAssignments[m.ExternalID]; ok {
-					delete(authz.UserAssignments, m.ExternalID)
-					_ = h.store.SaveAppAuthz(authz)
-				}
-			}
+		if _, ok := authz.UserAssignments[m.ExternalID]; ok && m.Provider == mapKey {
+			delete(authz.UserAssignments, m.ExternalID)
+			changed = true
 		}
 	}
-	h.audit("app_local_user_deleted", h.appActor(r), getClientIP(r), map[string]interface{}{"app_id": appID, "guid": guid, "actor_kind": h.appActorKind(r)})
-	jsonResp(w, map[string]string{"status": "deleted"}, http.StatusOK)
+	if changed {
+		if err := h.store.SaveAppAuthz(authz); err != nil {
+			log.Printf("[app] delete local user: clear role assignment failed app=%s guid=%s err=%v", appID, guid, err)
+			return errLocalAssignmentCleanup
+		}
+	}
+	return nil
 }
 
 // handleSetLocalUserPassword resets an app-local user's password.