docs(scim): document reinstatement exposure and add TODOS backlog

Note in README and design that lossless restore makes revocation
IdP-authoritative: a member revoked in the vault is re-activated on the next
sync if the IdP still shows them active, and a leaked token can reinstate any
previously-confirmed member. Document token-management audit events and the
omitted-vs-empty members and externalId-uniqueness semantics. Add TODOS.md
tracking the deferred follow-ups (config-gated denial tests, live Entra
validation, coverage edges, perf backlog, upstream ip_constant lints).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

TODOS.md

@@ -0,0 +1,134 @@
+# TODOS
+
+## SCIM
+
+### Config-gated denial tests (SCIM_ENABLED=false, ORG_GROUPS_ENABLED=false)
+
+**What:** Test the two config master switches from the denied side: a valid token
+against `SCIM_ENABLED=false` must get the uniform 401, and any Groups route with
+`ORG_GROUPS_ENABLED=false` must get the SCIM-enveloped 501.
+
+**Why:** These are the primary kill switches for the whole surface and the one
+authz-matrix case the build plan promised that is still missing.
+
+**Context:** `CONFIG` is a process-global `LazyLock` pinned by the `test-support`
+`#[ctor]` before main, so the disabled states cannot be tested in the same
+process as the enabled suite. Needs either a second test binary with its own
+ctor env, or an injectable check in `guard.rs` / `groups.rs`. See the
+`plan-delivery-gap-scim-disabled-authz-case` learning.
+
+**Effort:** M
+**Priority:** P1
+**Depends on:** None
+
+### Live Entra ID tenant validation
+
+**What:** Run the full lifecycle against a throwaway Entra tenant: Test
+Connection, assign user, create/update/deprovision/restore, group sync, token
+rotation. Follow docs/scim/README.md as written and fix any doc drift found.
+
+**Why:** Everything is verified against RFC 7643/7644 and observed Entra
+behaviour, but no real Entra sync has run against this build yet.
+
+**Context:** User-driven (needs a tenant). Never production. The rate limiter
+keys on `IP_HEADER` (`X-Real-IP`) - the deployment in front must set it.
+
+**Effort:** M
+**Priority:** P1
+**Depends on:** Branch pushed and deployed somewhere TLS-fronted
+
+### Mail-enabled invite failure path, end to end
+
+**What:** An integration test that runs `post_user` with mail enabled and a
+failing SMTP target, asserting the 500 plus the rollback outcome.
+
+**Why:** The rollback decision logic is unit-tested
+(`provisioning_rollback_spares_preexisting_users`), but the branch that calls it
+(send_invite failure inside `post_user`) still never executes under test.
+
+**Context:** Needs mail enabled in `CONFIG` (same process-global constraint as
+above) plus a fast-failing SMTP endpoint; naive versions are slow or flaky.
+Consider folding into the same second test binary as the config-gated tests.
+
+**Effort:** M
+**Priority:** P2
+**Depends on:** Config-gated denial tests (shared harness)
+
+### Coverage: error-envelope edges and manage HTTP surface
+
+**What:** Tests for the remaining actionable gaps from the ship coverage audit
+(82%): malformed-body 400 / oversized-body 413 envelopes, HTTP-level 429
+envelope, POST /Users missing/invalid email 400s, policy-blocked restore 400,
+GroupPatch path-less object form (Entra sends this shape), externalId HTTP
+filters, POST /Groups blank-name 400 and dup-externalId 409, and the manage
+endpoints driven over HTTP with AdminHeaders.
+
+**Why:** These are the branches where a regression would surface to Entra as a
+wrong status code or envelope, currently proven only by adjacent coverage.
+
+**Context:** All testable in the existing Rocket-local harness except the
+manage endpoints, which need an AdminHeaders fixture. Coverage map in the
+2026-07-19 ship PR body has the full gap list.
+
+**Effort:** M
+**Priority:** P2
+**Depends on:** None
+
+### Performance backlog for large orgs
+
+**What:** SQL-side pagination for /Users and /Groups lists, batched group
+member writes (resolve via `eq_any`, diff instead of delete-all+reinsert on
+PUT), and secondary indexes on `users_organizations(org_uuid, external_id)` and
+`groups(organizations_uuid, external_id)`.
+
+**Why:** Current implementation loads full member/group sets per list request
+and issues ~4N queries for an N-member group write. Fine at self-host scale
+(hundreds of members); wasteful for thousands.
+
+**Context:** Indexes diverge from upstream's no-secondary-index convention -
+decide deliberately. Review findings from 2026-07-19 have the details.
+
+**Effort:** L
+**Priority:** P3
+**Depends on:** None
+
+### Harden SCIM write edges surfaced by adversarial review
+
+**What:** Three lower-severity robustness gaps from the 2026-07-19 adversarial
+pass: (1) externalId uniqueness is check-then-set with no backing DB unique
+index, so concurrent writes could duplicate the correlation key; (2) a Group
+displayName > 100 chars or externalId > 300 chars from Entra hits the MySQL
+column limit and returns a 500 instead of a 400 invalidValue; (3) `scim_status`
+returns key metadata behind only an AdminHeaders session with no password/OTP
+re-auth, unlike generate/delete.
+
+**Why:** None are exploitable today (single Entra sync engine; strict-mode MySQL
+only; admin session required), but each is an unenforced invariant or an
+inconsistent guard that a future change could turn into a real bug.
+
+**Why not now:** (1) wants a migration adding a partial unique index across three
+dialects - real schema work, deferrable; (2) wants length validation in the
+Group handlers; (3) is a one-line guard tightening. Bundle them.
+
+**Effort:** M
+**Priority:** P2
+**Depends on:** None
+
+## Upstream hygiene
+
+### ip_constant clippy lints in http_client.rs tests
+
+**What:** 9 pre-existing `clippy::ip_constant` errors in upstream
+`src/http_client.rs` test code under `--all-targets` with the 1.96 toolchain.
+
+**Why:** Blocks running `cargo clippy --all-targets` clean locally; will bite
+if CI ever lints test targets.
+
+**Context:** Upstream code, untouched per branch discipline. Fix as a separate
+commit or PR upstream (`Ipv4Addr::LOCALHOST` instead of hand-coded addresses).
+
+**Effort:** S
+**Priority:** P3
+**Depends on:** None
+
+## Completed

docs/scim/README.md

@@ -23,11 +23,17 @@ Entra ID is the tested IdP. Design details and diagrams: [design.md](design.md).
 
    ```ini
    SCIM_ENABLED=true
+   # strongly recommended: without it, SCIM changes leave no audit trail
+   ORG_EVENTS_ENABLED=true
    # optional tuning
    SCIM_RATELIMIT_SECONDS=1
    SCIM_RATELIMIT_MAX_BURST=60
    ```
 
+   The server prints a startup warning if `SCIM_ENABLED` is set while
+   `ORG_EVENTS_ENABLED` is not, because every SCIM change would then be
+   invisible in the org event log.
+
 2. Generate the per-organization SCIM token (requires an org admin session;
    re-authenticates with your master password like API key rotation):
 
@@ -93,8 +99,24 @@ Entra ID is the tested IdP. Design details and diagrams: [design.md](design.md).
   rather than erroring the sync.
 - **SCIM user id** is the org membership id, not the account id. The same
   person in two orgs has two SCIM ids: SCIM is org-scoped by construction.
+- **externalId is unique per org** for both Users and Groups, on every write
+  path (create, PUT, PATCH). A write that would duplicate one is a 409
+  `uniqueness` conflict and changes nothing; re-asserting a resource's own
+  externalId succeeds.
+- **A Group PUT that omits `members` leaves the member set unchanged.** Only
+  an explicit `"members": []` clears the group, so a sparse non-Entra client
+  cannot wipe membership by accident (Entra always sends the full list).
+- **Deprovision from the IdP, not just the vault.** Because restore is lossless,
+  a member you revoke in the web vault is silently re-activated on the next sync
+  if the IdP still shows them active - `active: true` reinstates a previously
+  confirmed member to full access with no re-confirmation. To offboard someone,
+  unassign or disable them in Entra. Rotate the org's SCIM token if it may have
+  leaked: it can reinstate any member the org previously confirmed, not only
+  invite and deprovision.
 - Every SCIM change is written to the org event log (admin-visible) with the
-  synthetic actor `vaultwarden-scim-...` when `ORG_EVENTS_ENABLED=true`.
+  synthetic actor `vaultwarden-scim-...` when `ORG_EVENTS_ENABLED=true`. Token
+  generation, rotation, and deletion are logged too, under the acting admin's
+  own identity.
 
 ## Operational lifecycle
 

docs/scim/design.md

@@ -23,7 +23,7 @@ sequenceDiagram
     Entra->>Guard: POST /Users {userName, externalId, active}
     Guard-->>H: ScimToken{org}
     H->>DB: create shell User (if new), Membership status=Invited(0)
-    H->>Mail: send invite (rollback membership on failure)
+    H->>Mail: send invite (on failure roll back: new user removed entirely, existing user keeps account, membership goes)
     H->>DB: log event (SCIM actor)
     H-->>Entra: 201 Created {id = membership uuid}
 
@@ -146,6 +146,7 @@ org's ids, so the surface does not confirm what exists.
 ```mermaid
 flowchart LR
     subgraph scim [src/api/scim]
+        modm[mod.rs\nScimJson body guard, pagination,\nlist/location helpers, SCIM actor consts]
         guard[guard.rs\nScimToken]
         errm[error.rs\nSCIM envelope + catchers]
         filter[filter.rs\neq filter parser]
@@ -167,6 +168,9 @@ flowchart LR
     guard --> ratelimit
     guard --> cryptom
     guard --> models
+    usersm --> modm
+    groupsm --> modm
+    disc --> modm
     usersm --> patchm
     usersm --> filter
     usersm --> modelsm
@@ -191,14 +195,35 @@ surface itself can never mint or rotate its own credential.
 
 | Topic | Choice | Reason |
 |---|---|---|
-| DELETE /Users | revoke, not delete | preserves `akey`; restore is lossless; compromised token cannot destroy state |
+| DELETE /Users | revoke, not delete | preserves `akey`; restore is lossless; compromised token cannot destroy state (but see the reinstatement note below) |
 | Roles | not synced (always User) | `Custom` collapses to Manager in `MembershipType::from_str`; no honest round-trip |
 | userName/displayName updates | accepted, ignored | email is login identity; `user.name` is global to the person; erroring would fail every directory rename sync |
 | Group DELETE | real delete | groups carry no E2EE state |
 | Group members | diff add/remove; replace only on PUT/replace | Entra PATCHes diffs, including `members[value eq "..."]` removal paths |
+| Group PUT without `members` | member set unchanged | RFC 7644 permits either reading; keeping membership means a sparse client cannot wipe a group by accident. Explicit `[]` still clears |
+| externalId | unique per org, on every write path | it is the IdP correlation key; a duplicate would make filter lookups and later syncs target an arbitrary group. Users and Groups both 409 on conflict, self re-assertion allowed |
 | Groups when disabled | loud 501 | `ldap_import` silently skips; silence hides misconfiguration in the IdP |
 | Filter misses | empty 200 list | Entra Test Connection probes a random user; distinguishable misses enable enumeration |
 
+### Revocation is IdP-authoritative, in both directions
+
+Because restore is lossless, `active: true` is not just an onboarding signal - it
+**reinstates a revoked member to exactly the state they were revoked from**,
+including `Confirmed` with the wrapped org key intact and no re-confirmation. That
+is the intended deprovision/reprovision behaviour, but it has a consequence worth
+stating plainly:
+
+- **A member revoked in the web vault will be silently re-activated on the next
+  sync if the IdP still shows them active.** SCIM cannot tell a security-motivated
+  admin revocation apart from an Entra-driven one; the IdP is the source of truth.
+  Offboarding must therefore be driven from the IdP (unassign or disable the user
+  there), not only by revoking in the vault.
+- The revoke-only DELETE means a leaked SCIM token cannot *destroy* memberships,
+  but the same token **can reinstate** any member the org previously confirmed and
+  later revoked, gated only by organization policy. Treat the per-org token as an
+  access-reinstatement credential, not merely an invite/deprovision one, when
+  scoping its exposure. Rotate it (management endpoint) if it may have leaked.
+
 ## Test strategy
 
 All tests are inline (bin-only crate). The integration harness runs the real
@@ -212,6 +237,14 @@ never-dropped pool (sqlite WAL locking).
 
 Covered end to end: the full provision / deprovision / restore lifecycle at
 every status offset (`-126`, `-127`, `-128`), duplicate and last-owner
-conflicts, uniform-401 byte-equality, enumeration shape, Entra PATCH quirks
-(op casing, string booleans, path-less values, filter-path member removal),
-pagination edges, and the rate limiter.
+conflicts, uniform-401 byte-equality (including a disabled key row with the
+correct secret), enumeration shape, Entra PATCH quirks (op casing, string
+booleans, path-less values, filter-path member removal), pagination edges,
+the rate limiter, group externalId uniqueness on PUT and PATCH, and the
+omitted-versus-empty `members` distinction on group PUT. Token minting is
+exercised through the real management path: the tests mint via
+`manage::mint_scim_token` (the same function the endpoint calls), assert the
+round-trip through the guard, and assert rotation kills the previous token.
+The provisioning rollback rules are pinned directly: a pre-existing account
+survives a failed provision (only the new membership is removed), while a
+shell account created by the failed request is removed entirely.