diff --git a/SECURITY-AUDIT.md b/SECURITY-AUDIT.md index 0955b21..9f1f997 100644 --- a/SECURITY-AUDIT.md +++ b/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:` 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. diff --git a/docs/API.md b/docs/API.md index ebcb440..033cdaa 100644 --- a/docs/API.md +++ b/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 \ diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 77c526b..aaf74ce 100644 --- a/docs/CONFIGURATION.md +++ b/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`. | diff --git a/internal/handler/admin.go b/internal/handler/admin.go index 55dd345..dd9a583 100644 --- a/internal/handler/admin.go +++ b/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) { diff --git a/internal/handler/app_selfservice.go b/internal/handler/app_selfservice.go index 724e146..6f4b4c0 100644 --- a/internal/handler/app_selfservice.go +++ b/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. diff --git a/internal/handler/delete_user_m41_test.go b/internal/handler/delete_user_m41_test.go new file mode 100644 index 0000000..f90c9ca --- /dev/null +++ b/internal/handler/delete_user_m41_test.go @@ -0,0 +1,147 @@ +package handler + +import ( + "net/http" + "testing" +) + +// TestDeletedUserCanBeBootstrappedAgain is the M41 regression end to end: after +// an admin deletes a user, bootstrap must create the user again (it used to +// resolve the leftover local: mapping and "update" a user that no +// longer existed), and the new user must be able to sign in. +func TestDeletedUserCanBeBootstrappedAgain(t *testing.T) { + h, _ := testSetup(t) + adm := adminHeaders() + body := map[string]interface{}{ + "users": []map[string]interface{}{{ + "username": "carol", "password": "pass1234", "display_name": "Carol", "email": "carol", + }}, + } + + bootstrap := func() map[string]interface{} { + t.Helper() + w := doJSON(h, "POST", "/api/admin/bootstrap", body, adm) + if w.Code != 200 { + t.Fatalf("bootstrap: expected 200, got %d: %s", w.Code, w.Body.String()) + } + var resp struct { + Users []map[string]interface{} `json:"users"` + } + parseJSON(t, w, &resp) + if len(resp.Users) != 1 { + t.Fatalf("bootstrap: expected 1 user in the answer, got %v", resp.Users) + } + return resp.Users[0] + } + + first := bootstrap() + if first["created"] != true { + t.Fatalf("first bootstrap: expected created=true, got %v", first) + } + guid := first["guid"].(string) + + if w := doJSON(h, "DELETE", "/api/admin/users/"+guid, nil, adm); w.Code != 200 { + t.Fatalf("delete: expected 200, got %d: %s", w.Code, w.Body.String()) + } + + second := bootstrap() + if second["created"] != true { + t.Fatalf("bootstrap after delete: expected created=true, got %v", second) + } + if second["guid"] == guid { + t.Fatalf("bootstrap after delete reused the deleted GUID %s", guid) + } + + w := doJSON(h, "POST", "/api/auth/login", map[string]interface{}{ + "username": "carol", "password": "pass1234", + }, nil) + if w.Code != 200 { + t.Fatalf("login after re-bootstrap: expected 200, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestAdminDeleteOfLocalUserClearsRoleAssignment: the app keeps an app-local +// user's roles under its username. Deleting the user through the admin API must +// clear them, as DELETE /api/app/users/{guid} does (M14), or a user created +// again under that name inherits the old roles. +func TestAdminDeleteOfLocalUserClearsRoleAssignment(t *testing.T) { + h, s := testSetup(t) + adm := adminHeaders() + + w := doJSON(h, "POST", "/api/admin/apps", map[string]interface{}{ + "app_id": "desk", "audience": "desk", "allow_local_users": true, + }, adm) + var app map[string]interface{} + parseJSON(t, w, &app) + cred := basicAuth("desk", app["app_secret"].(string)) + + w = doJSON(h, "POST", "/api/app/users", map[string]interface{}{ + "username": "agent2", "password": "deskpass1", "roles": []string{"Supervisor"}, + }, cred) + if w.Code != http.StatusCreated { + t.Fatalf("create: %d %s", w.Code, w.Body.String()) + } + var u map[string]interface{} + parseJSON(t, w, &u) + + if w := doJSON(h, "DELETE", "/api/admin/users/"+u["guid"].(string), nil, adm); w.Code != http.StatusOK { + t.Fatalf("admin delete: %d %s", w.Code, w.Body.String()) + } + if w := doJSON(h, "POST", "/api/app/users", map[string]interface{}{ + "username": "agent2", "password": "deskpass2", + }, cred); w.Code != http.StatusCreated { + t.Fatalf("create again: %d %s", w.Code, w.Body.String()) + } + authz, err := s.GetAppAuthz("desk") + if err != nil { + t.Fatalf("authz: %v", err) + } + if roles, ok := authz.UserAssignments["agent2"]; ok { + t.Fatalf("the new agent2 inherited the deleted user's roles %v", roles) + } +} + +// TestAppDeleteOfLocalUserRevokesTokens: DELETE /api/app/users/{guid} revokes the +// user's tokens like the admin delete does, so a token issued before the delete +// stops working. +func TestAppDeleteOfLocalUserRevokesTokens(t *testing.T) { + h, _ := testSetup(t) + adm := adminHeaders() + + w := doJSON(h, "POST", "/api/admin/apps", map[string]interface{}{ + "app_id": "desk", "audience": "desk", "allow_local_users": true, + }, adm) + var app map[string]interface{} + parseJSON(t, w, &app) + cred := basicAuth("desk", app["app_secret"].(string)) + + w = doJSON(h, "POST", "/api/app/users", map[string]interface{}{"username": "agent3", "password": "deskpass1"}, cred) + if w.Code != http.StatusCreated { + t.Fatalf("create: %d %s", w.Code, w.Body.String()) + } + var u map[string]interface{} + parseJSON(t, w, &u) + + w = doJSON(h, "POST", "/api/auth/login", map[string]interface{}{ + "username": "agent3", "password": "deskpass1", "app_id": "desk", + }, nil) + if w.Code != http.StatusOK { + t.Fatalf("login: %d %s", w.Code, w.Body.String()) + } + var tok map[string]interface{} + parseJSON(t, w, &tok) + bearer := map[string]string{"Authorization": "Bearer " + tok["access_token"].(string)} + if w := doJSON(h, "GET", "/api/auth/userinfo", nil, bearer); w.Code != http.StatusOK { + t.Fatalf("userinfo before delete: %d %s", w.Code, w.Body.String()) + } + + if w := doJSON(h, "DELETE", "/api/app/users/"+u["guid"].(string), nil, cred); w.Code != http.StatusOK { + t.Fatalf("app delete: %d %s", w.Code, w.Body.String()) + } + if w := doJSON(h, "GET", "/api/auth/userinfo", nil, bearer); w.Code != http.StatusUnauthorized { + t.Fatalf("access token after delete: want 401, got %d %s", w.Code, w.Body.String()) + } + if w := doJSON(h, "POST", "/api/auth/refresh", map[string]interface{}{"refresh_token": tok["refresh_token"]}, nil); w.Code != http.StatusUnauthorized { + t.Fatalf("refresh after delete: want 401, got %d %s", w.Code, w.Body.String()) + } +} diff --git a/internal/store/bolt.go b/internal/store/bolt.go index df6da74..6ae2883 100644 --- a/internal/store/bolt.go +++ b/internal/store/bolt.go @@ -80,6 +80,9 @@ func OpenBolt(dataDir string) (*BoltStore, error) { // Prune reverse-index entries a pre-H14 SetIdentityMapping left stranded on a // previous owner. Cheap (one pass over the index) and idempotent. s.repairMappingIndex() + // Report forward mappings that still point at a deleted user; remove them + // only on opt-in (M41). + s.repairDanglingMappings() return s, nil } @@ -172,28 +175,8 @@ func (s *BoltStore) DeleteApp(appID string) error { if err := tx.Bucket(bucketUsers).Delete([]byte(guid)); err != nil { return err } - if data := tx.Bucket(bucketIdxMappingsByGUID).Get([]byte(guid)); data != nil { - var mappings []IdentityMapping - if json.Unmarshal(data, &mappings) == nil { - for _, m := range mappings { - mk := mappingKey(m.Provider, m.ExternalID) - // Only delete a forward entry this user still actually owns. - // The reverse index is derived state; if it ever drifts again, - // the H8 cascade must not delete a live mapping belonging to - // somebody else (H14). Postgres cascades by - // `WHERE user_guid IN (...)`, which is inherently owner-scoped — - // this makes Bolt identical. - if owner := tx.Bucket(bucketIdentityMappings).Get(mk); owner == nil || string(owner) != guid { - continue - } - if err := tx.Bucket(bucketIdentityMappings).Delete(mk); err != nil { - return err - } - } - } - if err := tx.Bucket(bucketIdxMappingsByGUID).Delete([]byte(guid)); err != nil { - return err - } + if err := deleteOwnedMappings(tx, guid); err != nil { + return err } } if err := tx.Bucket(bucketApps).Delete([]byte(appID)); err != nil { @@ -463,12 +446,54 @@ func (s *BoltStore) UpdateUser(u *User) error { }) } +// DeleteUser removes, in one transaction, the user row, the identity mappings the +// user still owns (forward entries and the reverse index), its global roles and +// its direct permissions. Previously only the user row went, so for example +// local: kept pointing at a GUID with no user (M41; see +// dangling_mappings.go). Tokens and sessions are revoked by the delete handlers +// (purgeDeletedUserState), and an app-local user's per-app role assignment is +// cleared by deleteLocalUser. func (s *BoltStore) DeleteUser(guid string) error { return s.update(func(tx *bolt.Tx) error { + if err := deleteOwnedMappings(tx, guid); err != nil { + return err + } + if err := tx.Bucket(bucketUserRoles).Delete([]byte(guid)); err != nil { + return err + } + if err := tx.Bucket(bucketUserPermissions).Delete([]byte(guid)); err != nil { + return err + } return tx.Bucket(bucketUsers).Delete([]byte(guid)) }) } +// deleteOwnedMappings deletes the forward identity mappings guid still owns and +// its reverse-index entry. Only a forward entry this user actually owns is +// deleted: the reverse index is derived state, and if it ever drifts again a +// delete cascade must not remove a live mapping belonging to somebody else (H14). +// Postgres deletes by `WHERE user_guid = ...`, which is inherently owner-scoped; +// this makes Bolt identical. +func deleteOwnedMappings(tx *bolt.Tx, guid string) error { + data := tx.Bucket(bucketIdxMappingsByGUID).Get([]byte(guid)) + if data == nil { + return nil + } + var mappings []IdentityMapping + if json.Unmarshal(data, &mappings) == nil { + for _, m := range mappings { + mk := mappingKey(m.Provider, m.ExternalID) + if owner := tx.Bucket(bucketIdentityMappings).Get(mk); owner == nil || string(owner) != guid { + continue + } + if err := tx.Bucket(bucketIdentityMappings).Delete(mk); err != nil { + return err + } + } + } + return tx.Bucket(bucketIdxMappingsByGUID).Delete([]byte(guid)) +} + func (s *BoltStore) ListUsers() ([]*User, error) { var users []*User err := s.view(func(tx *bolt.Tx) error { @@ -762,6 +787,146 @@ func (s *BoltStore) repairMappingIndex() { log.Printf("[store] mapping-index repair: pruned stale claims for %d user(s) (H14)", len(changes)) } +// repairDanglingMappings reports forward identity mappings whose owner GUID has +// no user record, which DeleteUser left behind before M41. It removes them only +// when the operator opts in (see danglingRepairEnabled). Every mapping is logged +// before anything is written. +func (s *BoltStore) repairDanglingMappings() { + type dangling struct { + key, guid string + m IdentityMapping + split bool // m is known from the reverse index + } + var found []dangling + _ = s.view(func(tx *bolt.Tx) error { + users := tx.Bucket(bucketUsers) + // The forward key is "provider:externalID" and both halves may contain + // ':' (M38); only the reverse index records the split exactly. + splits := mappingSplits(tx) + return tx.Bucket(bucketIdentityMappings).ForEach(func(k, v []byte) error { + if users.Get(v) == nil { + m, ok := splits[string(k)] + found = append(found, dangling{key: string(k), guid: string(v), m: m, split: ok}) + } + return nil + }) + }) + if len(found) == 0 { + return + } + for _, d := range found { + if d.split { + log.Printf("[store] dangling-mapping check: provider=%q external_id=%q points at deleted user %s (M41)", d.m.Provider, d.m.ExternalID, d.guid) + } else { + log.Printf("[store] dangling-mapping check: key=%q points at deleted user %s (M41)", d.key, d.guid) + } + } + if !danglingRepairEnabled() { + logDanglingReportOnly(len(found)) + return + } + removed := 0 + if err := s.update(func(tx *bolt.Tx) error { + fwd := tx.Bucket(bucketIdentityMappings) + var live map[string]bool + for _, d := range found { + // Re-check inside the write transaction: skip a key re-pointed since. + if owner := fwd.Get([]byte(d.key)); owner == nil || string(owner) != d.guid || tx.Bucket(bucketUsers).Get(owner) != nil { + continue + } + if err := fwd.Delete([]byte(d.key)); err != nil { + return err + } + if err := tx.Bucket(bucketIdxMappingsByGUID).Delete([]byte(d.guid)); err != nil { + return err + } + removed++ + if !strings.HasPrefix(d.key, "applocal:") { + continue + } + if !d.split { + log.Printf("[store] dangling-mapping repair: key=%q is app-local but cannot be split; check that app's role assignments by hand (M41)", d.key) + continue + } + if live == nil { + live = liveAssignmentKeys(tx) + } + if err := clearStaleLocalAssignment(tx, d.m, live); err != nil { + return err + } + } + return nil + }); err != nil { + log.Printf("[store] dangling-mapping repair FAILED: %v", err) + return + } + log.Printf("[store] dangling-mapping repair: SA_REPAIR_DANGLING_MAPPINGS=1, removed %d of %d mapping(s) (M41)", removed, len(found)) +} + +// liveAssignmentKeys returns every key under which an app's UserAssignments can +// match a live user: GUID, sAMAccountName and the external IDs of the user's +// mappings (see handler.userAssignmentKeys). +func liveAssignmentKeys(tx *bolt.Tx) map[string]bool { + live := map[string]bool{} + users := tx.Bucket(bucketUsers) + _ = users.ForEach(func(k, v []byte) error { + var u User + if json.Unmarshal(v, &u) == nil { + live[string(k)] = true + if u.SAMAccountName != "" { + live[u.SAMAccountName] = true + } + } + return nil + }) + _ = tx.Bucket(bucketIdxMappingsByGUID).ForEach(func(k, v []byte) error { + if users.Get(k) == nil { + return nil + } + var mappings []IdentityMapping + if json.Unmarshal(v, &mappings) == nil { + for _, m := range mappings { + live[m.ExternalID] = true + } + } + return nil + }) + return live +} + +// clearStaleLocalAssignment removes the role assignment that the app in m's +// "applocal:" provider keeps under the deleted user's username, so a user +// created again under that name does not inherit the old roles (M14, M41). The +// key is kept if a live user can still match it: assignment keys are shared with +// directory users. +func clearStaleLocalAssignment(tx *bolt.Tx, m IdentityMapping, live map[string]bool) error { + appID := strings.TrimPrefix(m.Provider, "applocal:") + if live[m.ExternalID] { + log.Printf("[store] dangling-mapping repair: app %q keeps the role assignment for %q, a live user still matches that name (M41)", appID, m.ExternalID) + return nil + } + b := tx.Bucket(bucketAppAuthz) + data := b.Get([]byte(appID)) + if data == nil { + return nil + } + var authz AppAuthz + if err := json.Unmarshal(data, &authz); err != nil { + log.Printf("[store] dangling-mapping repair: app %q authz unreadable, role assignment for %q not checked: %v (M41)", appID, m.ExternalID, err) + return nil + } + if _, ok := authz.UserAssignments[m.ExternalID]; !ok { + return nil + } + delete(authz.UserAssignments, m.ExternalID) + out, err := json.Marshal(&authz) + if err != nil { + return err + } + log.Printf("[store] dangling-mapping repair: cleared app %q role assignment for deleted app-local user %q (M41)", appID, m.ExternalID) + return b.Put([]byte(appID), out) +} + func (s *BoltStore) removeMappingFromIndex(tx *bolt.Tx, userGUID string, m IdentityMapping) error { var mappings []IdentityMapping data := tx.Bucket(bucketIdxMappingsByGUID).Get([]byte(userGUID)) diff --git a/internal/store/dangling_mappings.go b/internal/store/dangling_mappings.go new file mode 100644 index 0000000..716f6ff --- /dev/null +++ b/internal/store/dangling_mappings.go @@ -0,0 +1,34 @@ +package store + +import ( + "log" + "os" +) + +// Dangling identity mappings (M41). Before M41, DeleteUser removed only the user +// row, so the user's identity mappings kept pointing at a GUID with no user. +// Such a mapping keeps the username taken: bootstrap and LDAP import treat the +// user as existing, and app self-registration answers 409. It also keeps a +// deleted directory user out: an LDAP or Kerberos login resolves the mapping to +// the deleted GUID and fails, instead of provisioning a new account. +// +// Deployments may depend on that second effect without knowing it. Both stores +// therefore only REPORT dangling mappings on open by default, logging each one. +// They remove them all only when SA_REPAIR_DANGLING_MAPPINGS=1. For a removed +// app-local mapping, the repair also clears the app's role assignment for that +// username, unless a live user can still match the name. An operator can also +// remove mappings one at a time with DELETE +// /api/admin/users/{guid}/mappings/{provider}/{external_id}. Removing a +// directory mapping lets that user sign in again with a new account. +// SA_SKIP_MAPPING_REPAIR=1 overrides the opt-in, as it turns off the other +// startup mapping repair (H14). +func danglingRepairEnabled() bool { + return os.Getenv("SA_REPAIR_DANGLING_MAPPINGS") == "1" && os.Getenv("SA_SKIP_MAPPING_REPAIR") != "1" +} + +func logDanglingReportOnly(n int) { + log.Printf("[store] dangling-mapping check: %d mapping(s) point at deleted users and were left in place. "+ + "Removing an ldap or kerberos mapping lets that user sign in again with a new account. "+ + "Remove single mappings with DELETE /api/admin/users/{guid}/mappings/{provider}/{external_id}, "+ + "or restart with SA_REPAIR_DANGLING_MAPPINGS=1 to remove them all (M41)", n) +} diff --git a/internal/store/delete_user_test.go b/internal/store/delete_user_test.go new file mode 100644 index 0000000..68abf53 --- /dev/null +++ b/internal/store/delete_user_test.go @@ -0,0 +1,218 @@ +package store + +import ( + "testing" + + bolt "go.etcd.io/bbolt" +) + +// TestDeleteUserRemovesMappingsRolesAndPermissions is the M41 regression: a +// deleted user must not leave identity mappings, roles or direct permissions +// behind, so the same username can be provisioned again. +func TestDeleteUserRemovesMappingsRolesAndPermissions(t *testing.T) { + s := openTestBolt(t, t.TempDir()) + u := &User{DisplayName: "Alice"} + if err := s.CreateUser(u); err != nil { + t.Fatalf("create: %v", err) + } + if err := s.SetIdentityMapping("local", "alice", u.GUID); err != nil { + t.Fatalf("map local: %v", err) + } + if err := s.SetIdentityMapping("ldap", "alice", u.GUID); err != nil { + t.Fatalf("map ldap: %v", err) + } + if err := s.SetUserRoles(u.GUID, []string{"Analyst"}); err != nil { + t.Fatalf("roles: %v", err) + } + if err := s.SetUserPermissions(u.GUID, []string{"reports:read"}); err != nil { + t.Fatalf("permissions: %v", err) + } + + if err := s.DeleteUser(u.GUID); err != nil { + t.Fatalf("delete: %v", err) + } + + for _, p := range []string{"local", "ldap"} { + if guid, err := s.ResolveMapping(p, "alice"); err == nil { + t.Fatalf("%s:alice still resolves to %s after DeleteUser", p, guid) + } + } + if claims := indexClaims(t, s, u.GUID); len(claims) != 0 { + t.Fatalf("reverse index still holds %v", claims) + } + if roles, _ := s.GetUserRoles(u.GUID); len(roles) != 0 { + t.Fatalf("roles survived: %v", roles) + } + if perms, _ := s.GetUserPermissions(u.GUID); len(perms) != 0 { + t.Fatalf("permissions survived: %v", perms) + } + + // The username is free again. + u2 := &User{DisplayName: "Alice again"} + if err := s.CreateUser(u2); err != nil { + t.Fatalf("create again: %v", err) + } + if err := s.SetIdentityMapping("local", "alice", u2.GUID); err != nil { + t.Fatalf("map again: %v", err) + } + if guid, err := s.ResolveMapping("local", "alice"); err != nil || guid != u2.GUID { + t.Fatalf("local:alice = %q, %v; want %s", guid, err, u2.GUID) + } +} + +// TestDeleteUserKeepsMappingOwnedBySomeoneElse: if the reverse index ever claims +// a mapping that the forward bucket gives to another user, deleting the stale +// claimant must not delete the real owner's mapping (H14 applied to M41). +func TestDeleteUserKeepsMappingOwnedBySomeoneElse(t *testing.T) { + s := openTestBolt(t, t.TempDir()) + stale := &User{DisplayName: "Stale"} + owner := &User{DisplayName: "Owner"} + for _, u := range []*User{stale, owner} { + if err := s.CreateUser(u); err != nil { + t.Fatalf("create: %v", err) + } + } + if err := s.SetIdentityMapping("local", "erin", owner.GUID); err != nil { + t.Fatalf("map: %v", err) + } + if err := s.update(func(tx *bolt.Tx) error { + return s.addMappingToIndex(tx, stale.GUID, IdentityMapping{Provider: "local", ExternalID: "erin"}) + }); err != nil { + t.Fatalf("inject stale claim: %v", err) + } + + if err := s.DeleteUser(stale.GUID); err != nil { + t.Fatalf("delete: %v", err) + } + if guid, err := s.ResolveMapping("local", "erin"); err != nil || guid != owner.GUID { + t.Fatalf("local:erin = %q, %v; the owner's mapping must survive", guid, err) + } +} + +// danglingMappingStore creates a user mapped as local:, then removes only +// the user row, as DeleteUser did before M41. It returns the store's directory +// and the deleted GUID; the store is closed. +func danglingMappingStore(t *testing.T, name string) (string, string) { + t.Helper() + dir := t.TempDir() + s := openTestBolt(t, dir) + gone := &User{DisplayName: "Gone"} + if err := s.CreateUser(gone); err != nil { + t.Fatalf("create: %v", err) + } + if err := s.SetIdentityMapping("local", name, gone.GUID); err != nil { + t.Fatalf("map: %v", err) + } + if err := s.update(func(tx *bolt.Tx) error { + return tx.Bucket(bucketUsers).Delete([]byte(gone.GUID)) + }); err != nil { + t.Fatalf("simulate old delete: %v", err) + } + s.Close() + return dir, gone.GUID +} + +// TestDanglingMappingsReportOnlyByDefault: without the opt-in, opening the store +// reports a dangling mapping and leaves it in place. Removing it could let a +// deleted directory user sign in again. +func TestDanglingMappingsReportOnlyByDefault(t *testing.T) { + dir, guid := danglingMappingStore(t, "ivan") + s := openTestBolt(t, dir) + if got, err := s.ResolveMapping("local", "ivan"); err != nil || got != guid { + t.Fatalf("local:ivan = %q, %v; the default check must not write", got, err) + } +} + +// TestRepairDanglingMappingsOnOpen: with SA_REPAIR_DANGLING_MAPPINGS=1, mappings +// pointing at a deleted user are removed on open and live mappings are kept. +func TestRepairDanglingMappingsOnOpen(t *testing.T) { + dir, guid := danglingMappingStore(t, "frank") + s := openTestBolt(t, dir) + live := &User{DisplayName: "Live"} + if err := s.CreateUser(live); err != nil { + t.Fatalf("create: %v", err) + } + if err := s.SetIdentityMapping("local", "grace", live.GUID); err != nil { + t.Fatalf("map live: %v", err) + } + s.Close() + + t.Setenv("SA_REPAIR_DANGLING_MAPPINGS", "1") + s2 := openTestBolt(t, dir) + if got, err := s2.ResolveMapping("local", "frank"); err == nil { + t.Fatalf("dangling local:frank -> %s survived the repair", got) + } + if claims := indexClaims(t, s2, guid); len(claims) != 0 { + t.Fatalf("reverse index still holds %v for the deleted user", claims) + } + if got, err := s2.ResolveMapping("local", "grace"); err != nil || got != live.GUID { + t.Fatalf("live local:grace = %q, %v; must be kept", got, err) + } +} + +// TestDanglingRepairRespectsSkipEnv: SA_SKIP_MAPPING_REPAIR=1 overrides the +// opt-in, so nothing is written. +func TestDanglingRepairRespectsSkipEnv(t *testing.T) { + dir, guid := danglingMappingStore(t, "heidi") + t.Setenv("SA_REPAIR_DANGLING_MAPPINGS", "1") + t.Setenv("SA_SKIP_MAPPING_REPAIR", "1") + s := openTestBolt(t, dir) + if got, err := s.ResolveMapping("local", "heidi"); err != nil || got != guid { + t.Fatalf("local:heidi = %q, %v; SA_SKIP_MAPPING_REPAIR=1 must not write", got, err) + } +} + +// TestRepairDanglingClearsLocalAssignment: when the opt-in repair removes an +// app-local mapping, it also clears the app's role assignment for that username, +// unless a live user can still match the name. +func TestRepairDanglingClearsLocalAssignment(t *testing.T) { + dir := t.TempDir() + s := openTestBolt(t, dir) + gone1, gone2, live := &User{DisplayName: "Gone1"}, &User{DisplayName: "Gone2"}, &User{DisplayName: "Live"} + for _, u := range []*User{gone1, gone2, live} { + if err := s.CreateUser(u); err != nil { + t.Fatalf("create: %v", err) + } + } + for _, m := range []struct{ provider, name, guid string }{ + {"applocal:desk", "agent9", gone1.GUID}, + {"applocal:desk", "kim", gone2.GUID}, + {"local", "kim", live.GUID}, // a live user still matches "kim" + } { + if err := s.SetIdentityMapping(m.provider, m.name, m.guid); err != nil { + t.Fatalf("map: %v", err) + } + } + if err := s.SaveAppAuthz(&AppAuthz{AppID: "desk", UserAssignments: map[string][]string{ + "agent9": {"Supervisor"}, "kim": {"Viewer"}, + }}); err != nil { + t.Fatalf("authz: %v", err) + } + if err := s.update(func(tx *bolt.Tx) error { + for _, g := range []string{gone1.GUID, gone2.GUID} { + if err := tx.Bucket(bucketUsers).Delete([]byte(g)); err != nil { + return err + } + } + return nil + }); err != nil { + t.Fatalf("simulate old delete: %v", err) + } + s.Close() + + t.Setenv("SA_REPAIR_DANGLING_MAPPINGS", "1") + s2 := openTestBolt(t, dir) + authz, err := s2.GetAppAuthz("desk") + if err != nil { + t.Fatalf("authz: %v", err) + } + if roles, ok := authz.UserAssignments["agent9"]; ok { + t.Fatalf("stale assignment for agent9 survived: %v", roles) + } + if roles := authz.UserAssignments["kim"]; len(roles) != 1 { + t.Fatalf("assignment for kim, which a live user matches, must be kept; got %v", roles) + } + if _, err := s2.ResolveMapping("applocal:desk", "kim"); err == nil { + t.Fatal("dangling applocal:desk:kim survived the repair") + } +} diff --git a/internal/store/migrate.go b/internal/store/migrate.go index a9172b2..78fd495 100644 --- a/internal/store/migrate.go +++ b/internal/store/migrate.go @@ -519,8 +519,9 @@ func MigrateFromPostgres(source *PostgresStore, target *BoltStore, statusCh chan // TestPostgresConnection tests if a Postgres DSN is reachable. // If the target database doesn't exist, it auto-creates it. func TestPostgresConnection(dsn string) error { - // First try connecting directly - pg, err := OpenPostgres(dsn) + // First try connecting directly. Skip the dangling-mapping check: this only + // probes the DSN. + pg, err := openPostgres(dsn, false) if err == nil { pg.Close() return nil @@ -532,7 +533,7 @@ func TestPostgresConnection(dsn string) error { } // Retry after creation - pg, err = OpenPostgres(dsn) + pg, err = openPostgres(dsn, false) if err != nil { return err } diff --git a/internal/store/postgres.go b/internal/store/postgres.go index 9b1c711..5b72a5a 100644 --- a/internal/store/postgres.go +++ b/internal/store/postgres.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "io" + "log" "sort" "strings" "time" @@ -22,6 +23,12 @@ type PostgresStore struct { // OpenPostgres creates a new PostgresStore. dsn is a PostgreSQL connection string // (e.g. "postgres://user:pass@host:5432/dbname?sslmode=disable"). func OpenPostgres(dsn string) (*PostgresStore, error) { + return openPostgres(dsn, true) +} + +// openPostgres opens the store. checkMappings runs the dangling-mapping check; +// TestPostgresConnection turns it off because it only probes the DSN. +func openPostgres(dsn string, checkMappings bool) (*PostgresStore, error) { db, err := sql.Open("pgx", dsn) if err != nil { return nil, fmt.Errorf("open postgres: %w", err) @@ -42,9 +49,125 @@ func OpenPostgres(dsn string) (*PostgresStore, error) { db.Close() return nil, fmt.Errorf("migrate postgres schema: %w", err) } + if checkMappings { + // Report identity mappings that still point at a deleted user; remove + // them only on opt-in (M41). + s.repairDanglingMappings() + } return s, nil } +// repairDanglingMappings reports identity mappings whose user no longer exists, +// which DeleteUser left behind before M41. It removes them only when the +// operator opts in (see danglingRepairEnabled), and then deletes exactly the +// mappings it logged. +func (s *PostgresStore) repairDanglingMappings() { + type dangling struct{ provider, externalID, guid string } + rows, err := s.db.Query(`SELECT m.provider, m.external_id, m.user_guid FROM sa_identity_mappings m + WHERE NOT EXISTS (SELECT 1 FROM sa_users u WHERE u.guid = m.user_guid)`) + if err != nil { + log.Printf("[store] dangling-mapping check: query failed: %v", err) + return + } + var found []dangling + for rows.Next() { + var d dangling + if err := rows.Scan(&d.provider, &d.externalID, &d.guid); err != nil { + rows.Close() + log.Printf("[store] dangling-mapping check: scan failed: %v", err) + return + } + found = append(found, d) + } + err = rows.Err() + rows.Close() + if err != nil { + log.Printf("[store] dangling-mapping check: query failed: %v", err) + return + } + if len(found) == 0 { + return + } + for _, d := range found { + log.Printf("[store] dangling-mapping check: provider=%q external_id=%q points at deleted user %s (M41)", d.provider, d.externalID, d.guid) + } + if !danglingRepairEnabled() { + logDanglingReportOnly(len(found)) + return + } + tx, err := s.db.Begin() + if err != nil { + log.Printf("[store] dangling-mapping repair FAILED: %v", err) + return + } + defer tx.Rollback() + var removed int64 + for _, d := range found { + // Delete only the logged tuple, and only while its user is still missing. + res, err := tx.Exec(`DELETE FROM sa_identity_mappings m + WHERE m.provider = $1 AND m.external_id = $2 AND m.user_guid = $3 + AND NOT EXISTS (SELECT 1 FROM sa_users u WHERE u.guid = m.user_guid)`, d.provider, d.externalID, d.guid) + if err != nil { + log.Printf("[store] dangling-mapping repair FAILED: %v", err) + return + } + n, _ := res.RowsAffected() + removed += n + if n > 0 && strings.HasPrefix(d.provider, "applocal:") { + if err := clearStaleLocalAssignmentPG(tx, strings.TrimPrefix(d.provider, "applocal:"), d.externalID); err != nil { + log.Printf("[store] dangling-mapping repair FAILED: %v", err) + return + } + } + } + if err := tx.Commit(); err != nil { + log.Printf("[store] dangling-mapping repair FAILED: %v", err) + return + } + log.Printf("[store] dangling-mapping repair: SA_REPAIR_DANGLING_MAPPINGS=1, removed %d of %d mapping(s) (M41)", removed, len(found)) +} + +// clearStaleLocalAssignmentPG removes the role assignment appID keeps under a +// deleted app-local user's username, unless a live user can still match that +// key (see BoltStore clearStaleLocalAssignment). +func clearStaleLocalAssignmentPG(tx *sql.Tx, appID, username string) error { + var liveMatch bool + if err := tx.QueryRow(`SELECT EXISTS (SELECT 1 FROM sa_identity_mappings m JOIN sa_users u ON u.guid = m.user_guid WHERE m.external_id = $1) + OR EXISTS (SELECT 1 FROM sa_users WHERE guid = $1 OR data->>'sam_account_name' = $1)`, username).Scan(&liveMatch); err != nil { + return err + } + if liveMatch { + log.Printf("[store] dangling-mapping repair: app %q keeps the role assignment for %q, a live user still matches that name (M41)", appID, username) + return nil + } + var data []byte + err := tx.QueryRow(`SELECT data FROM sa_app_authz WHERE app_id = $1 FOR UPDATE`, appID).Scan(&data) + if err == sql.ErrNoRows { + return nil + } + if err != nil { + return err + } + var authz AppAuthz + if err := json.Unmarshal(data, &authz); err != nil { + log.Printf("[store] dangling-mapping repair: app %q authz unreadable, role assignment for %q not checked: %v (M41)", appID, username, err) + return nil + } + if _, ok := authz.UserAssignments[username]; !ok { + return nil + } + delete(authz.UserAssignments, username) + out, err := json.Marshal(&authz) + if err != nil { + return err + } + if _, err := tx.Exec(`UPDATE sa_app_authz SET data = $2 WHERE app_id = $1`, appID, out); err != nil { + return err + } + log.Printf("[store] dangling-mapping repair: cleared app %q role assignment for deleted app-local user %q (M41)", appID, username) + return nil +} + func (s *PostgresStore) migrate() error { schema := ` CREATE TABLE IF NOT EXISTS sa_users ( @@ -365,9 +488,25 @@ func (s *PostgresStore) UpdateUser(u *User) error { return nil } +// DeleteUser removes the user row, its identity mappings, its global roles and +// its direct permissions in one transaction (M41; see BoltStore.DeleteUser). func (s *PostgresStore) DeleteUser(guid string) error { - _, err := s.db.Exec(`DELETE FROM sa_users WHERE guid = $1`, guid) - return err + tx, err := s.db.Begin() + if err != nil { + return err + } + defer tx.Rollback() + for _, q := range []string{ + `DELETE FROM sa_identity_mappings WHERE user_guid = $1`, + `DELETE FROM sa_user_roles WHERE guid = $1`, + `DELETE FROM sa_user_permissions WHERE guid = $1`, + `DELETE FROM sa_users WHERE guid = $1`, + } { + if _, err := tx.Exec(q, guid); err != nil { + return err + } + } + return tx.Commit() } func (s *PostgresStore) ListUsers() ([]*User, error) { diff --git a/internal/store/postgres_delete_user_test.go b/internal/store/postgres_delete_user_test.go new file mode 100644 index 0000000..2919c69 --- /dev/null +++ b/internal/store/postgres_delete_user_test.go @@ -0,0 +1,198 @@ +package store + +import ( + "os" + "testing" + + "github.com/google/uuid" +) + +// The Postgres tests need a database: set SA_TEST_POSTGRES_DSN, e.g. +// postgres://postgres:postgres@127.0.0.1:5432/simpleauth_test?sslmode=disable +// They are skipped otherwise, so `go test ./...` still needs no external service. +func openTestPostgres(t *testing.T) *PostgresStore { + t.Helper() + dsn := os.Getenv("SA_TEST_POSTGRES_DSN") + if dsn == "" { + t.Skip("SA_TEST_POSTGRES_DSN not set") + } + s, err := OpenPostgres(dsn) + if err != nil { + t.Fatalf("OpenPostgres: %v", err) + } + t.Cleanup(func() { s.Close() }) + return s +} + +// cleanupPostgresUser removes what a test created for guid, so repeated runs +// against the same database start clean. +func cleanupPostgresUser(t *testing.T, s *PostgresStore, guid string) { + t.Cleanup(func() { + for _, q := range []string{ + `DELETE FROM sa_identity_mappings WHERE user_guid = $1`, + `DELETE FROM sa_user_roles WHERE guid = $1`, + `DELETE FROM sa_user_permissions WHERE guid = $1`, + `DELETE FROM sa_users WHERE guid = $1`, + } { + if _, err := s.db.Exec(q, guid); err != nil { + t.Errorf("cleanup %s: %v", guid, err) + } + } + }) +} + +// danglingPostgresMapping creates a user mapped as local:, then removes +// only the user row, as DeleteUser did before M41. It returns the deleted GUID. +func danglingPostgresMapping(t *testing.T, s *PostgresStore, name string) string { + t.Helper() + gone := &User{DisplayName: "Gone"} + if err := s.CreateUser(gone); err != nil { + t.Fatalf("create: %v", err) + } + cleanupPostgresUser(t, s, gone.GUID) + if err := s.SetIdentityMapping("local", name, gone.GUID); err != nil { + t.Fatalf("map: %v", err) + } + if _, err := s.db.Exec(`DELETE FROM sa_users WHERE guid = $1`, gone.GUID); err != nil { + t.Fatalf("simulate old delete: %v", err) + } + return gone.GUID +} + +// TestPostgresDeleteUserRemovesMappingsRolesAndPermissions is the M41 regression +// for the Postgres store. +func TestPostgresDeleteUserRemovesMappingsRolesAndPermissions(t *testing.T) { + s := openTestPostgres(t) + name := "m41-" + uuid.NewString() + u := &User{DisplayName: "Alice"} + if err := s.CreateUser(u); err != nil { + t.Fatalf("create: %v", err) + } + cleanupPostgresUser(t, s, u.GUID) + if err := s.SetIdentityMapping("local", name, u.GUID); err != nil { + t.Fatalf("map: %v", err) + } + if err := s.SetUserRoles(u.GUID, []string{"Analyst"}); err != nil { + t.Fatalf("roles: %v", err) + } + if err := s.SetUserPermissions(u.GUID, []string{"reports:read"}); err != nil { + t.Fatalf("permissions: %v", err) + } + + if err := s.DeleteUser(u.GUID); err != nil { + t.Fatalf("delete: %v", err) + } + if guid, err := s.ResolveMapping("local", name); err == nil { + t.Fatalf("local:%s still resolves to %s after DeleteUser", name, guid) + } + if roles, _ := s.GetUserRoles(u.GUID); len(roles) != 0 { + t.Fatalf("roles survived: %v", roles) + } + if perms, _ := s.GetUserPermissions(u.GUID); len(perms) != 0 { + t.Fatalf("permissions survived: %v", perms) + } +} + +// TestPostgresDanglingMappingsReportOnlyByDefault: without the opt-in, opening +// the store leaves a dangling mapping in place. +func TestPostgresDanglingMappingsReportOnlyByDefault(t *testing.T) { + s := openTestPostgres(t) + name := "m41-report-" + uuid.NewString() + guid := danglingPostgresMapping(t, s, name) + + s2 := openTestPostgres(t) + if got, err := s2.ResolveMapping("local", name); err != nil || got != guid { + t.Fatalf("local:%s = %q, %v; the default check must not write", name, got, err) + } +} + +// TestPostgresRepairDanglingMappingsOnOpen: with SA_REPAIR_DANGLING_MAPPINGS=1, a +// mapping left pointing at a deleted user is removed on open; a live mapping is +// kept. +func TestPostgresRepairDanglingMappingsOnOpen(t *testing.T) { + s := openTestPostgres(t) + goneName, liveName := "m41-gone-"+uuid.NewString(), "m41-live-"+uuid.NewString() + danglingPostgresMapping(t, s, goneName) + live := &User{DisplayName: "Live"} + if err := s.CreateUser(live); err != nil { + t.Fatalf("create: %v", err) + } + cleanupPostgresUser(t, s, live.GUID) + if err := s.SetIdentityMapping("local", liveName, live.GUID); err != nil { + t.Fatalf("map live: %v", err) + } + + t.Setenv("SA_REPAIR_DANGLING_MAPPINGS", "1") + s2 := openTestPostgres(t) + if guid, err := s2.ResolveMapping("local", goneName); err == nil { + t.Fatalf("dangling local:%s -> %s survived the repair", goneName, guid) + } + if guid, err := s2.ResolveMapping("local", liveName); err != nil || guid != live.GUID { + t.Fatalf("live local:%s = %q, %v; must be kept", liveName, guid, err) + } +} + +// TestPostgresConnectionDoesNotRepair: TestPostgresConnection only probes the +// DSN, so it must not remove anything even with the opt-in set. +func TestPostgresConnectionDoesNotRepair(t *testing.T) { + s := openTestPostgres(t) + name := "m41-probe-" + uuid.NewString() + guid := danglingPostgresMapping(t, s, name) + + t.Setenv("SA_REPAIR_DANGLING_MAPPINGS", "1") + if err := TestPostgresConnection(os.Getenv("SA_TEST_POSTGRES_DSN")); err != nil { + t.Fatalf("TestPostgresConnection: %v", err) + } + if got, err := s.ResolveMapping("local", name); err != nil || got != guid { + t.Fatalf("local:%s = %q, %v; TestPostgresConnection must not write", name, got, err) + } +} + +// TestPostgresRepairDanglingClearsLocalAssignment: the Postgres repair clears +// the app's role assignment for a removed app-local mapping, unless a live user +// can still match the name. +func TestPostgresRepairDanglingClearsLocalAssignment(t *testing.T) { + s := openTestPostgres(t) + appID := "m41-" + uuid.NewString() + goneName, sharedName := "agent-"+uuid.NewString(), "kim-"+uuid.NewString() + t.Cleanup(func() { s.db.Exec(`DELETE FROM sa_app_authz WHERE app_id = $1`, appID) }) + gone1 := &User{DisplayName: "Gone1"} + gone2 := &User{DisplayName: "Gone2"} + live := &User{DisplayName: "Live"} + for _, u := range []*User{gone1, gone2, live} { + if err := s.CreateUser(u); err != nil { + t.Fatalf("create: %v", err) + } + cleanupPostgresUser(t, s, u.GUID) + } + for _, m := range []struct{ provider, name, guid string }{ + {"applocal:" + appID, goneName, gone1.GUID}, + {"applocal:" + appID, sharedName, gone2.GUID}, + {"local", sharedName, live.GUID}, + } { + if err := s.SetIdentityMapping(m.provider, m.name, m.guid); err != nil { + t.Fatalf("map: %v", err) + } + } + if err := s.SaveAppAuthz(&AppAuthz{AppID: appID, UserAssignments: map[string][]string{ + goneName: {"Supervisor"}, sharedName: {"Viewer"}, + }}); err != nil { + t.Fatalf("authz: %v", err) + } + if _, err := s.db.Exec(`DELETE FROM sa_users WHERE guid = ANY($1)`, []string{gone1.GUID, gone2.GUID}); err != nil { + t.Fatalf("simulate old delete: %v", err) + } + + t.Setenv("SA_REPAIR_DANGLING_MAPPINGS", "1") + s2 := openTestPostgres(t) + authz, err := s2.GetAppAuthz(appID) + if err != nil { + t.Fatalf("authz: %v", err) + } + if roles, ok := authz.UserAssignments[goneName]; ok { + t.Fatalf("stale assignment for %s survived: %v", goneName, roles) + } + if roles := authz.UserAssignments[sharedName]; len(roles) != 1 { + t.Fatalf("assignment for %s, which a live user matches, must be kept; got %v", sharedName, roles) + } +}