diff --git a/TODOS.md b/TODOS.md new file mode 100644 index 00000000..125d8a16 --- /dev/null +++ b/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 diff --git a/docs/scim/README.md b/docs/scim/README.md index fcf471ff..d73152ba 100644 --- a/docs/scim/README.md +++ b/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 diff --git a/docs/scim/design.md b/docs/scim/design.md index b7993745..dcc7a87f 100644 --- a/docs/scim/design.md +++ b/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.