diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 987a97699..1bc2fcfd2 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -9419,6 +9419,7 @@ So this column does not close an unguarded hole. It replaces incidental, extra-g > **Filed 2026-08-08. RESEARCH item: the goal is an HONEST pass, and "cannot honestly reach pass" is a valid finding.** ASVS **8.2.2** (L1) currently scores **partial**. The pinned verb asks that data-specific access be restricted to consumers with explicit permissions to specific data items, to mitigate IDOR and BOLA. **As filed**, one PHI-bearing object family had no object-level authorization at all: `GET /uploads` listed every user's files unfiltered, and browse and delete took a `file_id` straight through to `get_meta`/`read_bytes` with no channel and no owner check. > **UPDATE 2026-08-11 -- THE UPLOADS HALF IS BUILT; THE ITEM STAYS OPEN.** Owner-only plus a `files:access_any` Administrator override, keyed on the immutable `uploader_id` (`Identity.user_id`), owner-ratified and recorded in [ADR 0134](adr/0134-offline-uploaded-logs-viewer-connection-decoupled-upload-browse-resend-deletion-phi-at-rest-posture-stdlib-multipart.md) Amendment A. All four routes now enforce it, including resend's SOURCE file (it previously checked only the TARGET inbound). So the three specific defects named above are fixed in the shipped code and the paragraph above describes the pre-fix state, not the current one. > **WHY IT IS NOT CLOSED, and none of these is a formality.** (1) The principal-narrowing axis is UNTOUCHED: `Identity.allowed_channels` still defaults to `None` (`auth/identity.py:38`) = every channel. (2) This item's own acceptance says **"Both halves need answers; neither alone closes the cell"** -- one half is answered. (3) ADR 0134 Amendment A satisfies only the SECOND clause of the master-test-plan's exit criterion 12; `GET /uploads` remains pageless, so the first clause is open. (4) The scorecard carrying the 8.2.2 verdict lives outside this repo and is not re-scored by this work. +> **UPDATE 2026-09-03 -- THE PRINCIPAL-NARROWING AND PAGINATION LIMBS ARE BUILT; THE ITEM STAYS OPEN.** Reasons (1) and (3) above are answered and describe the pre-fix state from here on. **Reason (3):** `GET /uploads` takes `limit`/`offset` (50, 1..500 / 0.., the same bounds declared on the `/ui` twin so the console door is not the looser one), `total` still counts the whole visible set, and the window is applied AFTER the owner filter so a page's length can never encode how many of another operator's files fell inside it. `UploadStore.list_files` sorts by `(uploaded_at, file_id)` because a timestamp is not a total order and a tied file could otherwise land on two pages or none. **The exit-criterion clause COULD NOT BE VERIFIED from an engine checkout, and that is the honest status.** Reason (3) above is phrased against the master test plan's exit criterion 12, and that document is **vaulted** -- `git ls-files docs/testing` returns exactly one file here, `docs/testing/VERIFY.md`, because ADR 0160 moved the plan out and the ignore rules make its absence look like non-existence rather than misplacement. So the pagination is BUILT and its own behaviour is tested; whether it closes that clause is a question only a reader holding the plan can answer, and nobody should re-derive an answer from this row. What is independently true, and is why the work stands on its own: an unbounded listing over a PHI-bearing family is worth fixing whatever the plan says, and a prior ruling already recorded the pageless listing as a resource-consumption gap rather than an object-authorization one -- so this limb was never going to clear the 8.2.2 cell either way. **Reasons (2) and (4) still stand,** and so do the four limbs this item names that nobody has built: the audit-actor immutable-id work, the connection-flag object check plus its by-id-AND-collection parity test, the metrics-exposition scoping, and the directory-immutable identity binding (`#1143`). Reason (1) detail follows. `Identity.allowed_channels` now defaults to the EMPTY set, `_allowed_channels` resolves a NULL `channel_scope` column to no channels rather than every channel, and all-channels survives as a grant somebody typed -- the `*` token in the stored scope list, reusing the string `ad_group_channels.channel` has always used for a wildcard row. The AD sync persists `["*"]` for a wildcard group instead of collapsing to NULL, which after the flip would have inverted a deliberate estate-wide mapping into a deny-everything one. Administrators stay all-channels by role, so the first operator of a fresh install is not locked out; the console's landing page carries the unprovisioned-operator sentence, deliberately a banner and not a start-time refusal. The store was not touched: `create_user` still writes no scope, and writing none is now what denies. **Reasons (2), (3) and (4) all still stand** -- the pagination limb is untouched, and the re-score is not this work's to make. > Verdict: research > Closing-act: scorecard-rescore @@ -9435,6 +9436,8 @@ So this column does not close an unguarded hole. It replaces incidental, extra-g **STILL UNANSWERED, and it is what keeps this item open:** narrowing `allowed_channels` by default carries a first-run cost, because new non-admins and unmapped AD users would see an empty console until scoped, which reads as broken RBAC on day one. Both halves need answers; neither alone closes the cell. +**ANSWERED 2026-09-03 -- the first-run cost is PAID, not avoided.** The default is narrowed and the cost is met where it lands: on the landing page, which tells an operator with an empty scope why the lists are empty and who fixes it, instead of leaving an unexplained empty console whose obvious remedy is to widen somebody's grant. It is deliberately a page banner and NOT a start-time refusal -- refusing to start would make a fresh single-operator install unbootable for exactly the same condition, which trades one first-run failure for a worse one. Administrators are all-channels by role, unchanged, so the first administrator of a fresh install is never locked out of their own console; that is the property the flip had to preserve and the one a test now pins. An unmapped AD user is denied and sees the same sentence, and the AD group map is the AD-native way to grant them a scope. Zero deployments (CLAUDE.md section 0) is why this is a flip and not a staged migration: there is nothing configured to break and nobody to notify, so the simple correct end state was available and was taken. + **What would NOT be an honest pass.** Flipping the `allowed_channels` default and rescoring. The triage is explicit that it narrows the channel axis and does not close the cell, because uploads carry no channel dimension -- the verdict would move on a change that does not touch the binding defect. Re-declaring uploaded files as something other than "data items" is the other tempting move and does not survive contact with the requirement's text or with the fact that those files carry message bodies. **Source:** filed 2026-08-08 from the ASVS ledger-coverage sweep, which found 80 of the 108 partial and fail cells carried no item naming them in `docs/BACKLOG.md`. The scorecard is the record of record for the verdict; this item tracks the research toward changing it. diff --git a/docs/SECURITY.md b/docs/SECURITY.md index b8ad1f5ee..3b8e7b2eb 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -192,8 +192,10 @@ route handler only when all of them pass. sensitive permission set on non-GET requests only. ADR 0118 relocated the knob, and the old `[diagnostics].audit_all_authz` TOML spelling is **refused at load**. 4. **A second axis: per-channel scope** — `users.channel_scope` narrows operational routes to a set of - connections. Out-of-scope *message* access returns **404** (existence-hiding); connection control and - inbound injection return **403**. Denials are audited `auth.channel_denied`. + connections, and it **denies by default**: a new non-administrator is granted no channel until + somebody grants one (BACKLOG #1152; the full rule is *Per-channel scoping (DLQ-SCOPE)* below). + Out-of-scope *message* access returns **404** (existence-hiding); connection control and inbound + injection return **403**. Denials are audited `auth.channel_denied`. The table below has **seven** rows. `require` is the ladder itself; five wrappers extend it (`require_paced`, `require_phi_read`, `require_step_up`, `require_step_up_action`, and the shared @@ -471,7 +473,7 @@ tuple: they act only on the caller's own account. | Method | Path | Permission | Gate | Extra constraints | |---|---|---|---|---| | `POST` | `/uploads` | `files:upload` | `require_step_up` | stdlib multipart parse (no `python-multipart`) | -| `GET` | `/uploads` | `files:browse` | `require` | metadata only — no body, no summary; **owner-scoped** (ASVS 8.2.2) — the caller sees only the files they uploaded unless they hold `files:access_any` | +| `GET` | `/uploads` | `files:browse` | `require` | metadata only — no body, no summary; **owner-scoped** (ASVS 8.2.2) — the caller sees only the files they uploaded unless they hold `files:access_any`; **paged** `limit`/`offset` (50, 1..500 / 0..), the window applied AFTER the owner filter so a page's length can never encode another operator's file count | | `GET` | `/uploads/{file_id}/messages` | `files:browse` | `require_step_up` | explicit `enforce_phi_read_hop` + `enforce_phi_read_pacing` (bulk decrypt + split); **owner-only** — another operator's file answers **404**, before the decrypt | | `POST` | `/uploads/{file_id}/messages/search` | `files:browse` | `require_step_up` | the needle-bearing sibling of the browse GET (BACKLOG #1184); same owner-only 404 before the decrypt, same bulk PHI-read pacing | | `POST` | `/uploads/{file_id}/resend` | `files:browse` | `require_step_up` | per-channel `can_access_channel` check on the target inbound (403) **and** an owner check on the source file (404) | @@ -498,10 +500,13 @@ tuple: they act only on the caller's own account. > **Owner-only** is the whole rule: list, browse, resend and delete reach the caller's own files. > `files:access_any` is the explicit cross-operator override, granted to **Administrator** only (it is > the whole catalogue), never to Operator, and never mintable onto a custom role -> (`CUSTOM_ROLE_FORBIDDEN_PERMISSIONS`). The channel axis is deliberately **not** used here — -> `Identity.allowed_channels` defaults to `null` (= every channel) and an uploaded file carries no -> channel, so a channel-scoped rule would protect nobody on a default install and would deny every -> scoped operator their own file. A denied by-id request answers **404** with the same body as a +> (`CUSTOM_ROLE_FORBIDDEN_PERMISSIONS`). The channel axis is deliberately **not** used here, and one +> of the two reasons originally given has since expired. The surviving reason decides it on its own: +> an uploaded file carries no channel, so a channel-scoped rule has nothing to match on and would +> deny every scoped operator their own file. The expired reason was that `Identity.allowed_channels` +> defaulted to `null` (= every channel), so such a rule would have protected nobody on a default +> install — BACKLOG #1152 flipped that default to deny, which changes nothing about the owner-only +> decision but does retire half of its stated justification. A denied by-id request answers **404** with the same body as a > malformed or absent id; what makes the by-id routes non-enumerable is that a `file_id` is 128 bits > of `secrets.token_hex(16)` and the listing no longer hands out another operator's — the denial is > still distinguishable by timing and by its audit row. That denial is audited as `upload.denied` with @@ -724,16 +729,26 @@ else would need its own authorization rule stated here. Differences 3–5 are derived and pinned: a `/ui` route that is weaker than **any** JSON route holding the same permission set on the same method reds CI until it is listed here. -> **Per-channel scoping (DLQ-SCOPE).** Operational permissions can be confined to a set of -> connections per user via `users.channel_scope` (`PUT /users/{id}/channel-scope`; `null` = all, -> the default). When a user is scoped, `messages:read/view_raw/replay`, dead-letter list/replay, and -> `connections:control` are restricted to their channels (out-of-scope message access returns 404 to -> avoid leaking existence; connection control returns 403; denials are audited `auth.channel_denied`). -> **Administrators are always all-channels.** Monitoring dashboards stay global. A channel-scoped user -> **cannot purge** a shared outbound (purge spans every inbound feeding it). **AD users** inherit their -> scope from the `ad_group_scope_map` (`GET/PUT /ad-group-scope-map`; channel `*` = all): on login the -> group-derived scope is persisted and stale sessions revoked. It's opt-in — with no matching mapped -> group, the user's existing scope (all by default) is left untouched. +> **Per-channel scoping (DLQ-SCOPE), and it DENIES BY DEFAULT (BACKLOG #1152, ASVS 8.2.2).** +> Operational permissions are confined to a set of connections per user via `users.channel_scope` +> (`PUT /users/{id}/channel-scope`). A new non-administrator is granted **no channel** — `create_user` +> writes no scope, and an absent scope denies — so `messages:read/view_raw/replay`, dead-letter +> list/replay and `connections:control` reach nothing until somebody grants a channel. Out-of-scope +> message access returns 404 to avoid leaking existence; connection control returns 403; denials are +> audited `auth.channel_denied`. All-channels survives as a grant somebody typed: the `*` token in the +> scope list (`{"channels": ["*"]}`). Sending `{"channels": null}` **clears** the scope and therefore +> denies — it is not the wide value it was before #1152. +> +> **Administrators are always all-channels**, by role, which is what keeps the first operator of a +> fresh install from locking themselves out of their own console. A non-administrator with an empty +> scope sees an empty console, and the landing page says so in a sentence rather than leaving it to +> read as broken RBAC; that is deliberately a page banner and not a start-time refusal, which would +> make a fresh single-operator install unbootable for the same condition. Monitoring dashboards stay +> global. A channel-scoped user **cannot purge** a shared outbound (purge spans every inbound feeding +> it). **AD users** inherit their scope from the `ad_group_scope_map` (`GET/PUT /ad-group-scope-map`; +> channel `*` = all): on login the group-derived scope is persisted — a wildcard row persists the +> explicit `["*"]` grant — and stale sessions revoked. It's opt-in: with no matching mapped group the +> user's existing scope is left untouched, which for a never-granted account means it stays denied. > **`/config/reload` executes Python** from the target directory in-process, so it is constrained > beyond the `config:deploy` permission: the directory must resolve **within** an allowed root — diff --git a/docs/adr/0134-offline-uploaded-logs-viewer-connection-decoupled-upload-browse-resend-deletion-phi-at-rest-posture-stdlib-multipart.md b/docs/adr/0134-offline-uploaded-logs-viewer-connection-decoupled-upload-browse-resend-deletion-phi-at-rest-posture-stdlib-multipart.md index 072076a2a..9c7c9a4b4 100644 --- a/docs/adr/0134-offline-uploaded-logs-viewer-connection-decoupled-upload-browse-resend-deletion-phi-at-rest-posture-stdlib-multipart.md +++ b/docs/adr/0134-offline-uploaded-logs-viewer-connection-decoupled-upload-browse-resend-deletion-phi-at-rest-posture-stdlib-multipart.md @@ -183,6 +183,15 @@ protect nobody on a default install while denying every scoped operator their ow protection depends on configuration nobody has set is a control resting on a false premise (CLAUDE.md §11, SDS-3.7). Owner-only protects with zero configuration. +> **Note added 2026-09-03 (BACKLOG #1152, the principal-narrowing limb).** The first clause above is no +> longer true of the code: `Identity.allowed_channels` now defaults to the EMPTY set, so an absent scope +> denies and the general lesson has been applied to the axis itself rather than only routed around. **The +> decision here is unchanged**, because the second clause was always sufficient on its own — an uploaded +> file carries no channel, so there is nothing for a channel rule to match and a scoped operator would be +> denied their own file. Recorded here rather than by editing the paragraph: a reader who checks the old +> premise against today's code must find out that it moved, not conclude that the owner-ratified decision +> lost its ground. + **Ownership is keyed on `uploader_id` (`Identity.user_id`), never on `username`.** A username is reassignable: deleting a local account and recreating the name is unblocked, and the AD leg auto-provisions a fresh row for a returning name with no administrator action at all. Keying an object-level control on it would mean that diff --git a/docs/adr/README.md b/docs/adr/README.md index d275fbab0..6f60d8711 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -162,7 +162,7 @@ what is withheld and what you can request. | [0131](0131-bulk-raw-message-body-export-from-a-search-result-step-up-audited-phi-egress.md) | **Bulk raw-message-body export from a search result (step-up, audited PHI egress)** (BACKLOG #124, demand-gate) — `GET /messages/export` on [`api/app.py`](../../messagefoundry/api/app.py) streams decrypted message bodies to a downloadable **NDJSON** file, the LARGEST PHI surface in the S7b cluster. Selection is an explicit `ids` set (*save-selected*) or the **basic** `/messages/search` filters (*save-all* — reusing `search_messages`), then it **LOOPS `get_message` per id** — **NO store schema change** (no 3-backend bulk iterator; `store_schema` stays false). Gated by `require_step_up(messages:export, messages:view_raw)` — a NEW **`messages:export`** capability distinct from view_raw (bulk ≠ opening one message), granted to OPERATOR + administrator — plus an explicit `enforce_phi_read_hop` ([ADR 0092](0092-posture-keyed-transport-hop-refusal-refuse-the-insecure-phi-hop.md)). **Per-row `_scope`** (`can_access_channel`) is re-checked on EVERY streamed body (load-bearing for the attacker-suppliable `ids` path; an out-of-scope id is skipped + `auth.channel_denied` audited). A single tamper-evident **`messages_export`** audit — actor + selection mode + basic filters + needle SHAPE (never the value, via `_search_audit_detail`) + the count of selected bodies — is written **BEFORE streaming** (mirroring `/audit/export`), so a scripted save-all can't harvest unaudited. `app.js` save-selected/save-all with a progress readout + stop control. PHI-safe destination is the operator's responsibility. Reuses [ADR 0046](0046-message-content-search.md); #26-clean | Accepted (2026-07-17) — DEMAND-GATE-BACKLOG Wave 3 (lane `dg-s7b`), pushes/PR owner-approved | | [0132](0132-per-endpoint-alternate-windows-credential-for-file-unc-shares-win32-ctypes-no-pywin32-no-impersonation-privilege.md) | **Per-endpoint alternate Windows credential for File/UNC shares (win32 ctypes, no pywin32, no impersonation privilege)** (BACKLOG #111, demand-gate) — a **File** endpoint may authenticate to a local/UNC (SMB) share under a Windows identity **distinct from the engine service account**, configured per-endpoint (`credential_username`/`credential_domain`/`credential_password`, the password **`env()`-only**, enforced by the `File()` factory and redacted via `_SECRET_SETTING_KEYS`). Established with **win32 ctypes** `advapi32.LogonUserW` (**`LOGON32_LOGON_NEW_CREDENTIALS`** — no privilege, no pywin32) + per-thread `ImpersonateLoggedOnUser`/`RevertToSelf`, mirroring the `tray/winsvc.py` + `service.py:124/270` + [ADR 0113](0113-windows-tray-service-manager-stdlib-ctypes-tokenless.md) precedent. A new `transports/wincred.CredentialContext` owns a **dedicated single-worker thread** and brackets every call `LogonUser→Impersonate→fn→RevertToSelf→CloseHandle` (per-call token → nothing leaks across reload; released on source `stop()` / dest `aclose()`). A single `_run_fs` **wraps** the S3a `_scan_once`/`validate_startup`/`_probe_dir_startup`/`after_read='leave'`/`_file_key`/ledger logic (ADR 0129) under the identity — the scan hook stays on the shared pool (unrelated AV/ICAP I/O). **Win32-only**: a non-Windows host raises `CredentialUnsupportedError` at build (loud, never a silent no-op — the CI-testable path); `CredentialLogonError` is an `OSError` so a bad credential rides the existing `except OSError` (DeliveryError / SourceStartupError / logged-retry, never a crash). A disjoint `POST /connections/{name}/test-credential` credentialed endpoint tester. Real UNC alt-cred is a Windows-CI/manual gate. Additive, default-off byte-identical; stdlib only. Wraps [ADR 0129](0129-process-in-place-file-disposition-and-cross-backend-processed-file-dedup-ledger.md); PHI-surface per [ADR 0092](0092-posture-keyed-transport-hop-refusal-refuse-the-insecure-phi-hop.md); #26-clean | Accepted (2026-07-18) — DEMAND-GATE-BACKLOG Wave 4 (lane `dg-s3b`), pushes/PR owner-approved | | [0133](0133-alert-escalation-tiers-schedule-aware-thresholds-and-content-triggered-alerts-the-56-remainder.md) | **Alert escalation tiers, schedule-aware thresholds, and content-triggered alerts (the #56 remainder)** (BACKLOG #81, demand-gate) — three additive capabilities on the [ADR 0014](0014-alerting-rules-engine.md) rules layer + [ADR 0044](0044-operator-alert-state.md) state, off by default. **Escalation tiers** (`AlertRule.escalate`) are **occurrence-driven** (NOT the ADR 0014 §3-declined timed chain): the notifier keeps a per-`(type,connection)` occurrence count and applies the highest tier whose `after_count` is reached (severity/transports/recipients override), persisted to a new `alert_instance.escalation_tier` column; any future timed re-eval sweep MUST be leader-gated. **Schedule-aware** rules reuse the #147/[ADR 0095](0095-connection-lifecycle-scheduler-and-credential-fault-stop.md) `Schedule` model verbatim — a rule matches only when `schedule.is_active(now)`. **Content-triggered (`content_match`)** alerts are a new PHI-FREE event (connection + operator label/rule id — **never the matched field value**) a code-first Handler emits off the routing hot path; the **purity/at-least-once carve-out** is reconciled by the existing **`(event_type, connection)` throttle/dedup** so a transform re-run's re-emit folds into the one instance (idempotent) + throttled notification. One additive `escalation_tier` column across SQLite/Postgres/SQL Server (SQL Server `COL_LENGTH`-gated ADD; Postgres `ADD COLUMN IF NOT EXISTS`) bumps the [ADR 0064](0064-schema-init-fastpath.md) schema hash; #26-clean | Accepted (2026-07-18) — DEMAND-GATE-BACKLOG Wave 4 (lane `dg-s1b`), pushes/PR owner-approved | -| [0134](0134-offline-uploaded-logs-viewer-connection-decoupled-upload-browse-resend-deletion-phi-at-rest-posture-stdlib-multipart.md) | **Offline uploaded-logs viewer — connection-decoupled upload/browse/per-message resend + deletion; PHI-at-rest posture; stdlib multipart** (BACKLOG #125 + #126, demand-gate) — an operator page to upload a partner-supplied `.hl7`/`.txt`/`.xml` file and browse it as a filterable/searchable offline log with per-message resend, decoupled from any live connection. Files are stored on the **filesystem** under an opt-in `[store].uploads_dir` (unset ⇒ every route 503s — no PHI surface unless configured), **AES-256-GCM-encrypted at rest under the store DEK** via the same `store/crypto.py` cipher (cell-AAD `("uploaded_file", "body"\|"meta", file_id)`; no-key ⇒ plaintext-on-disk, the File-connector spill-dir tier, documented in `docs/PHI.md` §2). Upload/browse/resend/delete are **deny-by-default** (`files:upload`/`files:browse`/`files:delete`, OPERATOR+admin) + **audited metadata-only**; browse (decrypts PHI bodies) additionally requires **step-up** + `enforce_phi_read_hop` ([ADR 0092](0092-posture-keyed-transport-hop-refusal-refuse-the-insecure-phi-hop.md)) and records the needle **shape** only ([ADR 0046](0046-message-content-search.md)). **Per-message resend uses a DISTINCT inject path — `store.enqueue_ingress(channel_id=…, raw=…)`, NOT `reingress`** ([ADR 0090](0090-resend-a-stored-message-to-an-alternate-outbound-connection.md)): `reingress` presupposes an origin `messages` row an uploaded file never had, whereas `enqueue_ingress` (the live listener's primitive) takes a running inbound channel directly and mints a fresh `RECEIVED` message. Multipart upload is **hand-parsed with stdlib** (`api/multipart.py`) — **no `python-multipart`** (honours the `routes/core.py` no-multipart stance; the 1 MiB body middleware is made path-aware for the upload cap). **#126 deletion (section under this ADR)**: confirm step + `upload.delete` audit + **path-traversal validation** — `file_id` must match `^[0-9a-f]{32}$` and resolve within the canonical uploads root or the route 404s without a filesystem touch. Console `/ui/uploaded-logs` reaches the engine via new `CoreHandlers` seam handlers (`ENGINE_UI_SEAM` bump). Extends [ADR 0001](0001-staged-pipeline-architecture.md) / [ADR 0019](0019-pluggable-keyprovider-hsm-kms-vault.md) / [ADR 0065](0065-web-ops-dashboard.md); #26-clean. **Amendment A (2026-08-11, owner-ratified):** object-level authorization (ASVS 8.2.2, BACKLOG #1152) - uploaded files are **owner-only** plus a `files:access_any` Administrator override never mintable onto a custom role; ownership keys on the **immutable `uploader_id` (`Identity.user_id`)**, never the reusable `username` (a recycled account name would otherwise inherit a departed operator's files); a missing `uploader_id` fails closed with no username fallback; the per-uploader quota re-keys to the same id so ownership and the budget cannot disagree; the channel axis was **rejected** because `allowed_channels` defaults to every channel and would protect nobody on a default install; the check sits in the handler **bodies**, not a `Depends` gate, because the console invokes them by reference across the seam | Accepted (2026-07-18) — DEMAND-GATE-BACKLOG Wave 5 (lane `dg-s8b`), pushes/PR owner-approved | +| [0134](0134-offline-uploaded-logs-viewer-connection-decoupled-upload-browse-resend-deletion-phi-at-rest-posture-stdlib-multipart.md) | **Offline uploaded-logs viewer — connection-decoupled upload/browse/per-message resend + deletion; PHI-at-rest posture; stdlib multipart** (BACKLOG #125 + #126, demand-gate) — an operator page to upload a partner-supplied `.hl7`/`.txt`/`.xml` file and browse it as a filterable/searchable offline log with per-message resend, decoupled from any live connection. Files are stored on the **filesystem** under an opt-in `[store].uploads_dir` (unset ⇒ every route 503s — no PHI surface unless configured), **AES-256-GCM-encrypted at rest under the store DEK** via the same `store/crypto.py` cipher (cell-AAD `("uploaded_file", "body"\|"meta", file_id)`; no-key ⇒ plaintext-on-disk, the File-connector spill-dir tier, documented in `docs/PHI.md` §2). Upload/browse/resend/delete are **deny-by-default** (`files:upload`/`files:browse`/`files:delete`, OPERATOR+admin) + **audited metadata-only**; browse (decrypts PHI bodies) additionally requires **step-up** + `enforce_phi_read_hop` ([ADR 0092](0092-posture-keyed-transport-hop-refusal-refuse-the-insecure-phi-hop.md)) and records the needle **shape** only ([ADR 0046](0046-message-content-search.md)). **Per-message resend uses a DISTINCT inject path — `store.enqueue_ingress(channel_id=…, raw=…)`, NOT `reingress`** ([ADR 0090](0090-resend-a-stored-message-to-an-alternate-outbound-connection.md)): `reingress` presupposes an origin `messages` row an uploaded file never had, whereas `enqueue_ingress` (the live listener's primitive) takes a running inbound channel directly and mints a fresh `RECEIVED` message. Multipart upload is **hand-parsed with stdlib** (`api/multipart.py`) — **no `python-multipart`** (honours the `routes/core.py` no-multipart stance; the 1 MiB body middleware is made path-aware for the upload cap). **#126 deletion (section under this ADR)**: confirm step + `upload.delete` audit + **path-traversal validation** — `file_id` must match `^[0-9a-f]{32}$` and resolve within the canonical uploads root or the route 404s without a filesystem touch. Console `/ui/uploaded-logs` reaches the engine via new `CoreHandlers` seam handlers (`ENGINE_UI_SEAM` bump). Extends [ADR 0001](0001-staged-pipeline-architecture.md) / [ADR 0019](0019-pluggable-keyprovider-hsm-kms-vault.md) / [ADR 0065](0065-web-ops-dashboard.md); #26-clean. **Amendment A (2026-08-11, owner-ratified):** object-level authorization (ASVS 8.2.2, BACKLOG #1152) - uploaded files are **owner-only** plus a `files:access_any` Administrator override never mintable onto a custom role; ownership keys on the **immutable `uploader_id` (`Identity.user_id`)**, never the reusable `username` (a recycled account name would otherwise inherit a departed operator's files); a missing `uploader_id` fails closed with no username fallback; the per-uploader quota re-keys to the same id so ownership and the budget cannot disagree; the channel axis was **rejected** because an uploaded file carries no channel to scope by (the second reason given at the time, that `allowed_channels` defaulted to every channel, expired when BACKLOG #1152 flipped that default to deny — see the note in the ADR); the check sits in the handler **bodies**, not a `Depends` gate, because the console invokes them by reference across the seam | Accepted (2026-07-18) — DEMAND-GATE-BACKLOG Wave 5 (lane `dg-s8b`), pushes/PR owner-approved | | [0135](0135-engine-brokered-ai-assistance-customer-managed-llm-egress-with-per-use-audit.md) | **Engine-brokered AI assistance — customer-managed LLM egress with per-use audit** (BACKLOG #95, demand-gate) — turn the reserved `[ai]` broker keys into a real integration: a NEW authenticated `POST /ai/chat` gated by `AI_ASSIST` that **re-resolves `resolve_effective_policy` server-side** (the server is the SOLE enforcement point — an IDE-claimed `data_scope` above the effective scope is **denied 403**, never honoured) and brokers the IDE assistant's call to a customer-managed / self-hosted LLM. **MVP boundary UNCHANGED: `code_only` regardless of mode** — never message bodies / PHI; an optional `AiMode.MANAGED_ENDPOINT` is kept **OUT** of `resolve_effective_policy`'s phi-granting branch (on-prem ≠ phi). **SSRF fail-closed:** a NEW broker in `transports/ai_broker.py` (mirrors `smart.py`, reuses `rest.py`'s hardened no-redirect `_NO_REDIRECT_OPENER` + `_redact_url`, runs off the event loop via `asyncio.to_thread`, **never imports `api/`**) validates the configured endpoint against a dedicated `[ai].allowed_endpoints` list — un-listed/empty ⇒ **REFUSE** — because `[egress].allowed_http` is permissive-when-empty and cannot be the gate. Broker credential `[ai].api_key` is **env-only** (`MEFOR_AI_API_KEY`, `_FILE_SECRET_KEYS` + `_SECRET_SETTING_KEYS`); prompts/responses/keys are **never** logged. **Per-use audit reuses the existing hash-chained `audit_log`** (`record_audit("ai.assist", …)`, PHI-safe metadata only — **no schema change**, no bespoke `ai_egress` table; `store_schema = false`). **No new dependency** (stdlib `urllib` via rest.py's opener — no vendor SDK). `ide/src/chat.ts` flips the managed path to the engine broker, keeping the `code_only` context. Extends [ADR 0024](0024-smart-backend-services-token-provider.md) / [ADR 0126](0126-outbound-forward-egress-web-proxy-for-the-stdlib-http-family.md); reuses [ADR 0092](0092-posture-keyed-transport-hop-refusal-refuse-the-insecure-phi-hop.md); #26-clean | Accepted (2026-07-18) — DEMAND-GATE-BACKLOG Wave 5 (lane `dg-s10`), pushes/PR owner-approved | | [0136](0136-per-user-saved-and-layered-log-search-filter-presets-extends-the-adr-0046-search-seam.md) | **Per-user saved & layered Log-Search filter presets** (BACKLOG #151, demand-gate) — save named content-search filter presets server-side per user, then recall + **layer** several into one combined query. A new **per-user `search_presets` table** across SQLite/Postgres/SQL Server (id-keyed; `UNIQUE(owner, name)`; save-by-name is an in-place replace preserving the id) whose PHI-shaped **`criteria` column is AES-256-GCM-encrypted at rest** through the store cipher (`cell_aad("search_presets", "criteria", id)`; added to each `_CIPHER_COLUMNS` so `rotate-key` re-encrypts it — id-keyed loop). Appending the DDL to the server `_SCHEMA` lists **moves `_schema_hash()`** — the [ADR 0064](0064-schema-init-fastpath.md) bump (no `_MIGRATION_REV` change); SQLite gets `CREATE TABLE IF NOT EXISTS`. Presets are **owner-scoped** (a user sees/recalls/deletes only their own); create + layered-recall are **step-up-gated + audited** (`preset.create`/`preset.layered_search`, needle **shape** only — never the value, reusing [ADR 0046](0046-message-content-search.md) `_needle_shape`), list/delete are `messages:read` + audited (no new permission). **Layering = bounded AND-compose over the typed `search_messages` params** (≤ 8 layers): metadata scalars take the first non-empty value and reject a conflicting second (400); **exactly one** content predicate is allowed (>1 or 0 → 400); the single needle builds a `SearchSpec` (ADR 0046 caps unchanged) and runs the **existing** `search_messages` (coupled with the S7b #124 basic-filter path — the composer stays bounded). The content term is loaded server-side from the encrypted column and **never round-trips to the client**, preserving the `routes/search.py` deliberate-drop posture. New `CoreHandlers` seam handlers (`ENGINE_UI_SEAM` bump) back the /ui save/recall/layer UI on the content-search page. Follows the [ADR 0045](0045-custom-rbac-roles.md) 3-backend precedent; extends [ADR 0046](0046-message-content-search.md); #26-clean | Accepted (2026-07-18) — DEMAND-GATE-BACKLOG Wave 5 (lane `dg-s8b`), pushes/PR owner-approved | | [0137](0137-time-boxed-retention-maintenance-pass-between-phase-cap.md) | **Time-boxed retention / log-maintenance pass — between-phase duration cap; VACUUM non-interruptible** (BACKLOG #121, demand-gate) — a light decision note over the existing `RetentionRunner`: a new `[retention].max_pass_seconds` (float; `0` = off, default) bounds the wall time one maintenance pass may spend. `run_once` captures a **monotonic** pass-start (injectable `monotonic=`, separate from the window `clock`) and, **before each phase** (purge → embedded-doc strip → prune → app-log sweep → WAL-checkpoint → VACUUM → size-check), checks the elapsed time; once the cap is reached the remaining phases are **skipped** for this pass and it is marked `RetentionPass.capped=True`. The deadline is checked **only between phases, never inside one**, so a **running `VACUUM` is non-interruptible** (SQLite VACUUM has no clean mid-flight abort — checked only *before* dispatch). A **skipped** WAL-checkpoint / VACUUM does **NOT** advance its `_last_wal` / `_last_vacuum_day` marker, so the next pass re-attempts it (defer, don't drop) — safe because every phase is idempotent. Metadata-only (no PHI); a capped pass is audited (`max_pass_seconds` + `capped` in the `retention_purge` detail, folded into `did_work`). Default `0` = byte-identical to the pre-#121 unbounded pass; recommended ~4 h (`14400`) when enabled (Corepoint default). Extends [ADR 0027](0027-per-connection-retention.md) / [ADR 0042](0042-embedded-document-pruning.md); sibling to [ADR 0130](0130-runtime-ephemeral-log-verbosity-control-and-phi-redacted-log-tail-viewer.md); #26-clean | Accepted (2026-07-19) — DEMAND-GATE-BACKLOG Wave 6 (lane `dg-s7a`), pushes/PR owner-approved | diff --git a/messagefoundry/api/_ui_seam.py b/messagefoundry/api/_ui_seam.py index f4069f051..020aefc4b 100644 --- a/messagefoundry/api/_ui_seam.py +++ b/messagefoundry/api/_ui_seam.py @@ -134,7 +134,7 @@ #: proof is that commit 40a4d5d9 added a REQUIRED ``UploadedFileList.scope`` field the console renders #: unconditionally while touching no seam file at all. Regenerate with #: ``python scripts/webconsole_seam_snapshot.py --write``; never hand-edit it to silence a gate. -ENGINE_UI_SEAM: str = "266cbfd342b22819" +ENGINE_UI_SEAM: str = "767521d4399d1ef4" @dataclass(frozen=True, slots=True) diff --git a/messagefoundry/api/app.py b/messagefoundry/api/app.py index e6648d36d..a28f37039 100644 --- a/messagefoundry/api/app.py +++ b/messagefoundry/api/app.py @@ -3944,9 +3944,14 @@ def _may_access_upload(identity: Identity, meta: UploadedFileMeta) -> bool: ``objectSid``. Closing it means binding AD to a directory-immutable id the way OIDC binds ``(issuer, sub)`` — tracked as BACKLOG #1143, not solvable inside this function. - The channel axis is deliberately NOT used: ``Identity.allowed_channels`` defaults to ``None`` - (= every channel) and an uploaded file carries no channel at all, so a channel-scoped rule - would protect nobody on a default install and would deny every scoped operator their own file. + The channel axis is deliberately NOT used, and ONE of its two original reasons has since + expired. The surviving one is decisive on its own: an uploaded file carries no channel at + all — it is decoupled from every connection by construction — so a channel-scoped rule has + nothing to match on and would deny every scoped operator their own file. The expired one was + that ``Identity.allowed_channels`` defaulted to ``None`` (= every channel), which would have + made such a rule protect nobody on a default install; BACKLOG #1152 flipped that default to + deny. Recorded rather than deleted, because a reader who remembers only the expired half + would think the owner-ratified decision (ADR 0134 Amendment A) had lost its ground. FAIL CLOSED on a sidecar with no ``uploader_id``. ``save()`` refuses to write one, but the tolerant loader yields ``""`` for a sidecar missing the key (a hand-placed one under the no-key @@ -4151,8 +4156,10 @@ async def list_uploaded_files( request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require(Permission.FILES_BROWSE)), + limit: int = Query(50, ge=1, le=500), + offset: int = Query(0, ge=0), ) -> UploadedFileList: - """List the caller's OWN uploaded files (metadata only — no bodies). Audited. + """List one page of the caller's OWN uploaded files (metadata only — no bodies). Audited. Object-level authorization (ASVS 8.2.2): the listing is owner-scoped, so one operator never sees another's filenames, sizes, digests or ``file_id`` s — the ``file_id`` being the token the @@ -4160,7 +4167,18 @@ async def list_uploaded_files( The scope is returned in the response AND recorded in the audit row — computed once, here — so a count means the same thing to every reader and no consumer has to re-derive whose files it is holding. The web console renders the sentence that matches it rather than asserting the - owner-scoped case at an override holder, for whom it is false.""" + owner-scoped case at an override holder, for whom it is false. + + **Paged (BACKLOG #1152).** The route was pageless, so one response carried every visible file + and its size grew with the age of the install — an uploads directory has no bound, and + ``list_files`` decrypts a sidecar per entry. ``total`` remains the whole visible count, which + is what the audit row and the console's "N file(s)" both mean; ``files`` is the window. + + **The window is applied AFTER the owner filter, and that order is the security-relevant + part.** Paging first would make each page's size depend on how many of another operator's + files happened to fall inside it, which turns the page length into a count of files the + caller may not know exist. Filter, then slice, and a scoped caller's pages are a function of + their own files only.""" us = _require_upload_store(request) # Filtered HERE, not in UploadStore.list_files(): the store's unscoped scan is what the # per-uploader quota and the age-based retention sweep are built on, and both must keep seeing @@ -4169,14 +4187,31 @@ async def list_uploaded_files( scope: Literal["own", "any_owner"] = ( "any_owner" if identity.has(Permission.FILES_ACCESS_ANY) else "own" ) + window = files[offset : offset + limit] await engine.store.record_audit( "upload.list", actor=identity.username, - detail=json.dumps({"count": len(files), "scope": scope}), + # `count` keeps meaning the whole visible set, unchanged from the pageless route, so an + # existing reader of this trail is not silently re-based onto a page size. `returned`, + # `limit` and `offset` are the new window. No filename and no owner: the audit of a + # listing is a count, not an inventory. + detail=json.dumps( + { + "count": len(files), + "returned": len(window), + "limit": limit, + "offset": offset, + "scope": scope, + } + ), client=client_ip(request), ) return UploadedFileList( - total=len(files), files=[_upload_info(m) for m in files], scope=scope + total=len(files), + files=[_upload_info(m) for m in window], + scope=scope, + limit=limit, + offset=offset, ) async def browse_uploaded_file( diff --git a/messagefoundry/api/auth_models.py b/messagefoundry/api/auth_models.py index 0b296127d..0c4993adc 100644 --- a/messagefoundry/api/auth_models.py +++ b/messagefoundry/api/auth_models.py @@ -64,7 +64,9 @@ class UserSummary(BaseModel): notify_email: str | None = None disabled: bool roles: list[str] - channel_scope: list[str] | None = None # per-channel RBAC: allowed connections; None = all + #: Per-channel RBAC, as STORED: the allowed connection names, ``["*"]`` for the explicit + #: all-channels grant, or ``None`` when nobody has set a scope — which denies (BACKLOG #1152). + channel_scope: list[str] | None = None class UserPermissions(BaseModel): @@ -81,7 +83,12 @@ class UserPermissions(BaseModel): class ChannelScope(BaseModel): - """A user's per-channel RBAC scope. ``None`` = all channels; a list = exactly those connections.""" + """A user's per-channel RBAC scope: a list of exactly those connections. + + ``["*"]`` is the explicit all-channels grant. ``None`` clears the scope back to unset, and unset + DENIES every channel (BACKLOG #1152, ASVS 8.2.2) — it is not the wide value it used to be, so a + client that sends null to widen a scope now narrows it to nothing. Administrators are + all-channels by role, so a scope set on one has no effect either way.""" channels: list[str] | None = Field(default=None, max_length=512) diff --git a/messagefoundry/api/auth_routes.py b/messagefoundry/api/auth_routes.py index b01d3055c..50c611720 100644 --- a/messagefoundry/api/auth_routes.py +++ b/messagefoundry/api/auth_routes.py @@ -177,7 +177,11 @@ def _login_response( def _parse_channel_scope(raw: str | None) -> list[str] | None: - """Decode the stored ``channel_scope`` JSON to a list (None = all; malformed → empty list).""" + """Decode the stored ``channel_scope`` JSON to a list, or ``None`` when no scope is stored. + + This is the STORED shape, not the resolved one: ``auth.service._allowed_channels`` is what turns + it into an access decision, and since BACKLOG #1152 a ``None`` here denies rather than widens. + Malformed JSON decodes to the empty list, which denies the same way.""" if raw is None: return None try: @@ -869,8 +873,10 @@ async def set_channel_scope( service: AuthService = Depends(_service), identity: Identity = Depends(require_step_up(Permission.USERS_MANAGE)), ) -> SimpleMessage: - """Set a user's per-channel RBAC scope (``channels: null`` = all). Administrators are always - all-channels, so a scope set on one has no effect.""" + """Set a user's per-channel RBAC scope. ``channels: ["*"]`` grants every channel; + ``channels: null`` clears the scope, which DENIES every channel (BACKLOG #1152 — null used + to be the wide value). Administrators are always all-channels, so a scope set on one has no + effect.""" if await service.store.get_user(user_id) is None: raise HTTPException(status.HTTP_404_NOT_FOUND, "no such user") await service.set_channel_scope(user_id, body.channels, actor=identity.username) diff --git a/messagefoundry/api/models.py b/messagefoundry/api/models.py index 4f8f7a929..735fafb31 100644 --- a/messagefoundry/api/models.py +++ b/messagefoundry/api/models.py @@ -1241,11 +1241,22 @@ class UploadedFileList(BaseModel): ``any_owner`` when the caller holds ``files:access_any``. It is the same value the ``upload.list`` audit row records, computed once at the route — so a reader of the response and a reader of the audit interpret the same count the same way, and a UI can state which listing it is showing - instead of asserting one of the two unconditionally. It is a fixed enum, never operator text.""" + instead of asserting one of the two unconditionally. It is a fixed enum, never operator text. + + **``total`` is the whole visible set; ``files`` is one page of it (BACKLOG #1152).** The two were + the same number while the route was pageless, and a consumer that assumed ``len(files) == + total`` is now wrong — which is the point: an uploads directory grows without bound, and a + listing that renders all of it makes response size a function of how long the install has been + running. ``limit`` and ``offset`` echo the window the route actually applied after clamping, so a + client paginates off the response rather than off the request it hoped was honoured.""" total: int files: list[UploadedFileInfo] scope: Literal["own", "any_owner"] + #: The page window, as APPLIED. Defaulted so a client built against the pageless shape still + #: decodes this model; the route always sets both. + limit: int = 50 + offset: int = 0 class UploadedMessageSummary(BaseModel): diff --git a/messagefoundry/api/security.py b/messagefoundry/api/security.py index 10a92dd86..02c814c9e 100644 --- a/messagefoundry/api/security.py +++ b/messagefoundry/api/security.py @@ -53,8 +53,16 @@ ) # Identity used when auth is explicitly disabled via allow_no_auth (embedding/dev): full access. +# allowed_channels=None is EXPLICIT and load-bearing: the field defaults to the empty set (deny) +# since BACKLOG #1152, and this identity exists precisely to stand in for "authorization is off", +# so it must carry the whole estate rather than inherit the deny-by-default an unprovisioned +# operator gets. _SYSTEM_IDENTITY = Identity.build( - user_id="system", username="system", auth_provider=AuthProvider.LOCAL, roles=list(Role) + user_id="system", + username="system", + auth_provider=AuthProvider.LOCAL, + roles=list(Role), + allowed_channels=None, ) # While an account is flagged to rotate its password, only these self-service routes stay reachable. diff --git a/messagefoundry/apiclient/client.py b/messagefoundry/apiclient/client.py index 9ec94707c..da613b2e1 100644 --- a/messagefoundry/apiclient/client.py +++ b/messagefoundry/apiclient/client.py @@ -821,11 +821,13 @@ def set_user_roles(self, user_id: str, roles: list[str]) -> None: self._request("PUT", f"/users/{_seg(user_id)}/roles", json={"roles": roles}) def get_channel_scope(self, user_id: str) -> list[str] | None: - """A user's per-channel RBAC scope (``None`` = all channels).""" + """A user's per-channel RBAC scope: the granted connections, ``["*"]`` for every channel, or + ``None`` when no scope is stored — which denies (BACKLOG #1152).""" return _decode(self._get(f"/users/{_seg(user_id)}/channel-scope"), ChannelScope).channels def set_channel_scope(self, user_id: str, channels: list[str] | None) -> None: - """Set a user's per-channel RBAC scope (``None`` = all channels).""" + """Set a user's per-channel RBAC scope. ``["*"]`` grants every channel; ``None`` clears the + scope and therefore DENIES every channel — it is not the wide value it used to be.""" self._request("PUT", f"/users/{_seg(user_id)}/channel-scope", json={"channels": channels}) def delete_user(self, user_id: str) -> None: diff --git a/messagefoundry/auth/identity.py b/messagefoundry/auth/identity.py index a6cc39e78..0dfcf8022 100644 --- a/messagefoundry/auth/identity.py +++ b/messagefoundry/auth/identity.py @@ -23,6 +23,15 @@ class AuthProvider(str, Enum): # noqa: UP042 AD = "ad" +#: The explicit all-channels grant token, as it is stored in ``users.channel_scope`` (the JSON list +#: ``["*"]``) and as the admin setter accepts it. BACKLOG #1152 (ASVS 8.2.2) retired the older +#: encoding where an ABSENT scope meant every channel: all-channels is now a grant somebody typed, +#: and saying nothing denies. The token is not new vocabulary — the AD-group-to-channel map has +#: always stored ``*`` for a wildcard row (``ad_group_channels.channel``), and this reuses it so one +#: string means one thing across the whole scope surface. +ALL_CHANNELS = "*" + + @dataclass(frozen=True, slots=True) class Identity: """An authenticated user with roles resolved to a flat, deny-by-default permission set.""" @@ -33,20 +42,28 @@ class Identity: roles: frozenset[Role] permissions: frozenset[Permission] must_change_password: bool = False - #: Per-channel RBAC scope: connections this user's *operational* permissions apply to. ``None`` - #: = all channels (the default / Administrators). + #: Per-channel RBAC scope: connections this user's *operational* permissions apply to. A + #: frozenset restricts to exactly those connection ids; ``None`` is unrestricted. Note ``None`` + #: and an EMPTY frozenset are not the same value -- None is every channel, and the empty set is + #: no channel at all. The first draft of this comment said "empty frozenset means every + #: channel", which is the inverse; it was caught by re-reading the annotation. + #: + #: **THE DEFAULT DENIES (BACKLOG #1152, ASVS 8.2.2), and it used to be ``None``.** An identity + #: built without a scope now reaches no channel, so a rule written against this axis protects a + #: default install instead of resting on a premise that was false the moment nobody typed + #: anything. Unrestricted is still reachable and is now always deliberate: the ADMINISTRATOR + #: role, or the :data:`ALL_CHANNELS` grant in the stored scope, both resolved by + #: ``auth.service._allowed_channels``. A caller constructing an ``Identity`` DIRECTLY (rather + #: than through the resolver) and wanting the whole estate must pass ``allowed_channels=None`` + #: and say why; ``api.security._SYSTEM_IDENTITY`` is one such site, and it says why there. #: #: BACKLOG #1151 (ASVS 8.1.1): this used to end "See docs/security/PHASE-8C-RBAC.md", which a #: reader of the public repository CANNOT REACH -- `docs/security/` is gitignored here, so #: `git ls-files docs/security` returns zero and the directory is absent from a fresh checkout. #: That is a standing decision, not an oversight, which is exactly why the pointer had to go: a #: dangling reference to a security document is worse than no reference, because it tells the - #: reader the rule is written down somewhere they can look. THE RULE ITSELF IS STATED ABOVE - #: INSTEAD: ``None`` is every channel; a frozenset restricts to exactly those connection ids. - #: Note ``None`` and an EMPTY frozenset are not the same value -- None is unrestricted, and an - #: empty set would restrict to nothing. The first draft of this comment said "empty frozenset - #: means every channel", which is the inverse; it was caught by re-reading the annotation. - allowed_channels: frozenset[str] | None = None + #: reader the rule is written down somewhere they can look. THE RULE ITSELF IS STATED ABOVE. + allowed_channels: frozenset[str] | None = frozenset() @classmethod def build( @@ -57,11 +74,14 @@ def build( auth_provider: AuthProvider, roles: Iterable[Role], must_change_password: bool = False, - allowed_channels: frozenset[str] | None = None, + allowed_channels: frozenset[str] | None = frozenset(), extra_permissions: Iterable[Permission] = (), ) -> Identity: """Construct an identity, resolving ``roles`` to their union of permissions. + ``allowed_channels`` defaults to the EMPTY set, not ``None`` — see the field's own note. Omit + it and the identity reaches no channel; pass ``None`` to mean the whole estate. + ``extra_permissions`` are unioned on top of the built-in role permissions — the additive custom-role overlay (ADR 0045): a user's effective set is *built-in-role ∪ custom-role* permissions. The flat ``permissions`` set is what every authorization check consults, so where @@ -84,7 +104,19 @@ def has(self, permission: Permission) -> bool: return permission in self.permissions def can_access_channel(self, channel_id: str | None) -> bool: - """True iff the user's per-channel scope permits ``channel_id`` (``None`` scope = all).""" + """True iff the user's per-channel scope permits ``channel_id``. + + A ``None`` scope is the whole estate; any frozenset permits exactly its members, so the + empty set — the default since BACKLOG #1152 — permits nothing.""" if self.allowed_channels is None: return True return channel_id is not None and channel_id in self.allowed_channels + + @property + def has_no_channels(self) -> bool: + """True iff this identity is scoped to zero connections — the unprovisioned-operator state. + + The console asks this to explain an empty page rather than let it read as broken RBAC on a + fresh install (BACKLOG #1152). It is a display question, never an authorization one: every + gate calls :meth:`can_access_channel`, which denies on exactly this state anyway.""" + return self.allowed_channels is not None and not self.allowed_channels diff --git a/messagefoundry/auth/permissions.py b/messagefoundry/auth/permissions.py index 95f41a48b..ebd71be7e 100644 --- a/messagefoundry/auth/permissions.py +++ b/messagefoundry/auth/permissions.py @@ -65,9 +65,13 @@ class Permission(str, Enum): # noqa: UP042 FILES_BROWSE = "files:browse" # list/browse/resend an uploaded file's messages (PHI read) FILES_DELETE = "files:delete" # delete an uploaded file from the server (destructive) # Object-level override for the uploaded-file subsystem (ASVS 8.2.2). Uploaded files are OWNER-ONLY: - # `files:browse`/`files:delete` reach only the files the caller uploaded, because the channel axis - # (Identity.allowed_channels) defaults to "every channel" and so would protect nobody on a default - # install. This permission widens that object scope to every uploader's files, for administrative + # `files:browse`/`files:delete` reach only the files the caller uploaded. The channel axis cannot do + # this job at all -- an uploaded file is decoupled from any connection and so carries no channel to + # scope by, which is why ownership is the resource attribute (ADR 0134 Amendment A, owner-ratified). + # The rejection was ALSO argued at the time from `Identity.allowed_channels` defaulting to every + # channel; BACKLOG #1152 has since made that default DENY, so only the first reason still stands and + # the owner-only decision is unchanged by the flip. This permission widens that object scope to every + # uploader's files, for administrative # oversight and cleanup after a departed operator. It is NOT a capability of its own — a holder # still needs `files:browse` / `files:delete` for the route itself — and no built-in role but # ADMINISTRATOR (which is the whole catalogue) grants it. Never assignable to a custom role diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 903843362..2e496106c 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -26,7 +26,7 @@ from uuid import uuid4 from messagefoundry.auth import oidc, reconcile, totp, webauthn -from messagefoundry.auth.identity import AuthProvider, Identity +from messagefoundry.auth.identity import ALL_CHANNELS, AuthProvider, Identity from messagefoundry.auth.ldap import AdPrincipal, LdapAuthenticator, LdapError, kerberos_principal from messagefoundry.auth.notifications import ( ACCOUNT_DISABLED, @@ -233,18 +233,29 @@ def _json(obj: Any) -> str: def _allowed_channels(user: UserRecord, roles: frozenset[Role]) -> frozenset[str] | None: - """Resolve a user's per-channel RBAC scope to a frozenset, or ``None`` for all channels. - - Administrators are always all-channels. A NULL ``channel_scope`` is all; a JSON list is exactly - those connections; anything malformed is treated as **no** channels (deny-by-default).""" - if Role.ADMINISTRATOR in roles or user.channel_scope is None: + """Resolve a user's stored per-channel RBAC scope to a frozenset, or ``None`` for all channels. + + **An ABSENT scope denies (BACKLOG #1152, ASVS 8.2.2).** ``create_user``'s INSERT does not list + ``channel_scope``, so every account is minted with SQL NULL there; NULL used to resolve to + ``None`` = every channel, which made every per-channel check in the API narrow nobody on a + default install. It now resolves to the empty set, so a freshly minted non-administrator reaches + no connection until an administrator grants one. + + Unrestricted is still reachable, and both ways are a deliberate grant: the ADMINISTRATOR role, + or :data:`~messagefoundry.auth.identity.ALL_CHANNELS` present in the stored list. A JSON list is + otherwise exactly those connections, and anything malformed is no channels.""" + if Role.ADMINISTRATOR in roles: return None + if user.channel_scope is None: + return frozenset() try: names = json.loads(user.channel_scope) except (ValueError, TypeError): return frozenset() if not isinstance(names, list): return frozenset() + if ALL_CHANNELS in names: + return None return frozenset(str(n) for n in names) @@ -1384,15 +1395,21 @@ async def _sync_ad_channel_scope( ) -> UserRecord: """Persist a user's AD-group-derived per-channel scope (C3) so it's durable for later requests (mirrors role sync). Administrators are always all-channels. If no group mapping - matches, the per-user scope is left untouched — opt-in, so it never clobbers a manual scope - or the all-channels default. Returns the (possibly refreshed) user record.""" + matches, the per-user scope is left untouched — opt-in, so it never clobbers a manual scope, + and since BACKLOG #1152 an untouched scope is a DENY rather than the whole estate. Returns + the (possibly refreshed) user record. + + A wildcard group row persists the explicit ``["*"]`` grant. It used to persist SQL NULL and + rely on NULL meaning "all"; with an absent scope now denying, that collapse would have + inverted a deliberate all-channels mapping into a deny-everything one.""" if Role.ADMINISTRATOR in roles: return user channels = await self._store.channels_for_ad_groups(groups) if not channels: return user - specific = sorted(c for c in channels if c != "*") - scope_json = None if "*" in channels else _json(specific) + wildcard = ALL_CHANNELS in channels + specific = sorted(c for c in channels if c != ALL_CHANNELS) + scope_json = _json([ALL_CHANNELS]) if wildcard else _json(specific) if user.channel_scope == scope_json: return user await self._store.set_user_channel_scope(user.id, scope_json) @@ -1402,7 +1419,7 @@ async def _sync_ad_channel_scope( await self._audit( "auth.ad_scope_resynced", actor=user.username, - detail=_json({"channels": "*" if scope_json is None else specific}), + detail=_json({"channels": ALL_CHANNELS if wildcard else specific}), ) return await self._store.get_user(user.id) or user @@ -3117,8 +3134,14 @@ async def admin_reset_password(self, user_id: str, *, actor: str) -> str: async def set_channel_scope( self, user_id: str, channels: Sequence[str] | None, *, actor: str ) -> None: - """Set a user's per-channel RBAC scope (``None`` = all). Revokes their sessions so the new - scope takes effect immediately, and audits the change.""" + """Set a user's per-channel RBAC scope. Revokes their sessions so the new scope takes effect + immediately, and audits the change. + + Three writable states, and ``None`` is no longer the wide one (BACKLOG #1152): ``None`` + clears the scope back to unset, which now DENIES every channel; ``[]`` denies too, and says + somebody chose it; a list containing + :data:`~messagefoundry.auth.identity.ALL_CHANNELS` grants the whole estate. Administrators + are all-channels by role, so a scope set on one still has no effect.""" scope_json = None if channels is None else _json(sorted(set(channels))) await self._store.set_user_channel_scope(user_id, scope_json) await self._store.revoke_user_sessions(user_id) diff --git a/messagefoundry/store/store.py b/messagefoundry/store/store.py index f742c6ff3..2883d6bed 100644 --- a/messagefoundry/store/store.py +++ b/messagefoundry/store/store.py @@ -839,9 +839,10 @@ class UserRecord: must_change_password: bool failed_attempts: int locked_until: float | None - channel_scope: str | None = ( - None # JSON list of allowed connection names; None = all (per-channel RBAC) - ) + # Per-channel RBAC, as stored: a JSON list of granted connection names, `["*"]` for every + # channel, or None when nobody has set one — which DENIES (BACKLOG #1152). The resolution lives + # in auth.service._allowed_channels; this column carries the grant, never the decision. + channel_scope: str | None = None # MFA (WP-14): whether a native TOTP second factor is enrolled+active, and when. The secret and # the recovery-code hashes are deliberately NOT carried here (least exposure) — they are read only # via the store's get_totp_secret / get_recovery_code_hashes accessors. @@ -1851,7 +1852,7 @@ def _append_channel_scope( must_change_password INTEGER NOT NULL DEFAULT 0, failed_attempts INTEGER NOT NULL DEFAULT 0, locked_until REAL, - channel_scope TEXT, -- per-channel RBAC: JSON list of connections; NULL = all + channel_scope TEXT, -- per-channel RBAC: JSON list of granted connections ('["*"]' = all); NULL = none granted, which denies (BACKLOG #1152) totp_secret TEXT, -- MFA (WP-14): base32 TOTP secret, store-cipher encrypted; NULL = none totp_enabled INTEGER NOT NULL DEFAULT 0, -- TOTP enrolled + confirmed active totp_enrolled_at REAL, @@ -8620,8 +8621,8 @@ async def set_user_roles( async def set_user_channel_scope( self, user_id: str, scope_json: str | None, *, now: float | None = None ) -> None: - """Set a user's per-channel scope. ``scope_json`` is a JSON list of connection names, or - ``None`` for all channels (per-channel RBAC).""" + """Set a user's per-channel scope. ``scope_json`` is a JSON list of granted connection names + (``'["*"]'`` for every channel), or ``None`` to clear it — which denies (BACKLOG #1152).""" now = time.time() if now is None else now async with self._lock: await self._db.execute( diff --git a/messagefoundry/uploads.py b/messagefoundry/uploads.py index d79f2d188..84772ee73 100644 --- a/messagefoundry/uploads.py +++ b/messagefoundry/uploads.py @@ -652,11 +652,17 @@ def _observed_sync(self, uploader_id: str) -> tuple[int, int]: async def list_files(self) -> list[UploadedFileMeta]: """List all uploaded files (newest first). Undecryptable/foreign sidecars are skipped with a - warning (never a body in the log), so a rotated-away key can't 500 the whole page.""" + warning (never a body in the log), so a rotated-away key can't 500 the whole page. + + The sort is TOTAL — ``file_id`` breaks a timestamp tie — because ``GET /uploads`` pages off + this order (BACKLOG #1152). ``uploaded_at`` alone is not a total order: files written in the + same instant compare equal, and the underlying scan is a directory walk whose order is not + guaranteed to repeat, so two requests could place a tied file on both pages or on neither. + A stable tiebreak is what makes "page 2" mean the same thing twice.""" def _scan() -> list[UploadedFileMeta]: out = self._scan_metas_sync() - out.sort(key=lambda m: m.uploaded_at, reverse=True) + out.sort(key=lambda m: (m.uploaded_at, m.file_id), reverse=True) return out return await asyncio.to_thread(_scan) diff --git a/messagefoundry_webconsole/__init__.py b/messagefoundry_webconsole/__init__.py index e98767b2f..64a525098 100644 --- a/messagefoundry_webconsole/__init__.py +++ b/messagefoundry_webconsole/__init__.py @@ -45,7 +45,7 @@ # If cross-seam support is ever genuinely wanted, re-widen this set AND add the CI matrix that # installs the MIN and MAX supported engine builds — the claim and its test land together, or not # at all. -SUPPORTED_ENGINE_SEAMS: frozenset[str] = frozenset({"266cbfd342b22819"}) +SUPPORTED_ENGINE_SEAMS: frozenset[str] = frozenset({"767521d4399d1ef4"}) #: The vendored static assets shipped in THIS wheel (mounted at /ui/static by :func:`mount_ui`). STATIC_DIR = Path(__file__).parent / "static" diff --git a/messagefoundry_webconsole/pages/admin.py b/messagefoundry_webconsole/pages/admin.py index c956b47b5..48d087370 100644 --- a/messagefoundry_webconsole/pages/admin.py +++ b/messagefoundry_webconsole/pages/admin.py @@ -21,6 +21,8 @@ RoleInfo, UserSummary, ) +from messagefoundry.auth.identity import ALL_CHANNELS +from messagefoundry.auth.permissions import Role from .._html import Markup, el, page, register_nav, rows_table from ._common import _seg @@ -56,6 +58,31 @@ def _admin_links(active_page: str) -> Markup: # --- users -------------------------------------------------------------------- +def _scope_cell(user: UserSummary) -> str: + """The Channel scope column for one user: what that account can actually REACH. + + Two things decide it and this column has to show the resolved answer, not one input. The stored + scope is the grant; ``None`` used to render "all" and, since BACKLOG #1152, an unset scope denies + — so it renders "(none)" alongside an explicit empty list, and all-channels is the ``*`` grant. + But an ADMINISTRATOR is all-channels BY ROLE whatever is stored (``_allowed_channels`` returns + before it reads the column), so rendering their stored scope would print "(none)" beside an + account holding the whole estate. + + Both directions of that error are worth naming, because they fail differently. Showing "all" for + an account that reaches nothing sends an administrator hunting a permission bug that is not + there. Showing "(none)" for an account that reaches everything hides real access from the person + whose job is to review it, which is the worse of the two. + """ + if Role.ADMINISTRATOR.value in user.roles: + return "all (administrator)" + stored = user.channel_scope + if stored is None: + return "(none)" + if ALL_CHANNELS in stored: + return "all" + return ", ".join(stored) or "(none)" + + def users_page(users: Sequence[UserSummary]) -> Markup: """The user list: every account with provider, roles, scope, and status; links to the admin forms.""" rows: list[list[object]] = [] @@ -67,7 +94,7 @@ def users_page(users: Sequence[UserSummary]) -> Markup: u.display_name or "", u.email or "", ", ".join(u.roles), - "all" if u.channel_scope is None else ", ".join(u.channel_scope) or "(none)", + _scope_cell(u), "disabled" if u.disabled else "active", ] ) @@ -192,12 +219,22 @@ def user_detail_page( action=f"/ui/users/{user.id}/roles", class_="ctl", ) - # Three explicit scope states so a deny-all ([]) scope ROUND-TRIPS: an empty textarea alone is - # ambiguous between "all channels" (None) and "no channels" ([]), and silently widening a stored - # deny-all to all-channels on a re-save would be a privilege-widening bug (review PR2-M3). - scope_mode = ( - "all" if user.channel_scope is None else ("none" if user.channel_scope == [] else "list") - ) + # Three explicit scope states so a deny-all scope ROUND-TRIPS: an empty textarea alone cannot + # say which of them was meant, and silently widening a stored deny-all to all-channels on a + # re-save would be a privilege-widening bug (review PR2-M3). + # + # BACKLOG #1152 moved which stored value is the wide one. All-channels is now the explicit + # ALL_CHANNELS grant; a null scope is "never granted" and denies, the same as []. Both null and + # [] therefore land on "none" — they differ only in whether anyone has touched the field, which + # is a provenance question the audit log answers and this form must not pretend to. + stored = user.channel_scope + listed = [c for c in (stored or []) if c != ALL_CHANNELS] + if stored is not None and ALL_CHANNELS in stored: + scope_mode = "all" + elif listed: + scope_mode = "list" + else: + scope_mode = "none" mode_options = [ el("option", label, value=value, selected=value == scope_mode or None) for value, label in ( @@ -212,7 +249,13 @@ def user_detail_page( el( "label", "Allowed connections (one per line)", - el("textarea", "\n".join(user.channel_scope or []), name="channels", rows=4), + el("textarea", "\n".join(listed), name="channels", rows=4), + ), + el( + "p", + "A new account starts with no channels and sees an empty console until you grant " + "some. Administrators reach every channel regardless of this setting.", + class_="muted", ), el("button", "Save channel scope", type="submit"), method="post", diff --git a/messagefoundry_webconsole/pages/connections.py b/messagefoundry_webconsole/pages/connections.py index d25940c64..a9eb5b53a 100644 --- a/messagefoundry_webconsole/pages/connections.py +++ b/messagefoundry_webconsole/pages/connections.py @@ -20,6 +20,7 @@ from ._common import _num, _secs, _seg __all__ = [ + "UNPROVISIONED_NOTICE", "bulk_control_result", "connection_details", "connections_fragment", @@ -29,6 +30,7 @@ "purge_confirm", "purge_pending", "purge_result", + "unprovisioned_banner", ] @@ -308,9 +310,36 @@ def _flagged_only_toggle() -> Markup: ) -def dashboard(rows: list[ConnectionRow]) -> Markup: +#: Shown on the landing page to an operator whose per-channel scope is empty (BACKLOG #1152). +#: A fresh non-administrator is granted no channel, so every list on this console is legitimately +#: empty for them. Without this sentence that state is indistinguishable from a broken install, and +#: an operator who reads it as broken RBAC files a bug or -- worse -- gets handed a wider grant to +#: make the symptom go away. It says what happened, who fixes it, and nothing about the estate: it +#: must not name a connection the reader is not authorized to know exists. +UNPROVISIONED_NOTICE = ( + "Your account is not granted access to any connection yet, so these lists are empty. " + "Ask an administrator to set your channel scope." +) + + +def unprovisioned_banner(unprovisioned: bool) -> Markup: + """The :data:`UNPROVISIONED_NOTICE` alert, or nothing. + + ``role="alert"`` so it reaches a screen reader that never sees the empty table below it.""" + if not unprovisioned: + return Markup("") + return el("p", UNPROVISIONED_NOTICE, class_="banner", role="alert") + + +def dashboard(rows: list[ConnectionRow], *, unprovisioned: bool = False) -> Markup: """The connections dashboard page; the table auto-refreshes via the first-party poll script. + ``unprovisioned`` raises :data:`UNPROVISIONED_NOTICE` above the table — the first-run affordance + for an operator with an empty channel scope (BACKLOG #1152). It is DELIBERATELY a page banner + and not a start-time refusal: refusing to start would make a fresh single-operator install + unbootable for the same condition, which is a worse answer to the same question. It defaults + False so every other caller renders byte-identically. + ``data-poll`` names the same-origin fragment endpoint; ``app.js`` fetches it every ``data-poll-ms`` and replaces this container's content with the server-rendered, already-escaped fragment. The ``#livestats`` strip is filled live by ``app.js`` over the ``/ws/stats`` WebSocket (M-ws); it @@ -329,6 +358,10 @@ def dashboard(rows: list[ConnectionRow]) -> Markup: "Connections", # Header + the filter/bulk-action toolbar on one row. el("div", el("h1", "Connections"), _controls_toolbar(), class_="page-head"), + # Outside [data-poll]: the 5s fragment swap replaces the table only, and this notice must + # survive it — an operator who scrolls past the first refresh must still see why the page + # is empty. + unprovisioned_banner(unprovisioned), livestats, live, active="dashboard", diff --git a/messagefoundry_webconsole/pages/uploaded_logs.py b/messagefoundry_webconsole/pages/uploaded_logs.py index 36893f332..6359625b5 100644 --- a/messagefoundry_webconsole/pages/uploaded_logs.py +++ b/messagefoundry_webconsole/pages/uploaded_logs.py @@ -92,12 +92,47 @@ def uploaded_logs(data: UploadedFileList, *, error: str = "") -> Markup: ["File", "Uploaded by", "Format", "Size", "Messages", "When", ""], rows, ), - el("p", text(f"{data.total} file(s)"), class_="pager"), + _list_pager(data), ] ) return page("Uploaded logs", *parts, active="uploaded-logs") +def _list_pager(data: UploadedFileList) -> Markup: + """The page counter plus Previous/Next links for the uploaded-files listing (BACKLOG #1152). + + ``data.total`` is the whole visible set and ``data.files`` is the window, so the line states + both rather than a bare count that would now be ambiguous. The links carry only two integers the + engine has already clamped, so nothing operator-supplied reaches the URL — the same reason the + refusal codes on this page are fixed tokens.""" + shown = len(data.files) + first = data.offset + 1 if shown else 0 + parts: list[object] = [text(f"{first}-{data.offset + shown} of {data.total} file(s)")] + if data.offset > 0: + prev = max(data.offset - data.limit, 0) + parts.append(Markup(" ")) + parts.append( + el( + "a", + "← Previous", + href=f"/ui/uploaded-logs?{urlencode({'limit': data.limit, 'offset': prev})}", + class_="btn-link", + ) + ) + if data.offset + shown < data.total: + nxt = data.offset + data.limit + parts.append(Markup(" ")) + parts.append( + el( + "a", + "Next →", + href=f"/ui/uploaded-logs?{urlencode({'limit': data.limit, 'offset': nxt})}", + class_="btn-link", + ) + ) + return el("p", *parts, class_="pager") + + def uploaded_logs_upload(*, error: str = "") -> Markup: """The upload form (multipart/form-data — hand-parsed server-side, ADR 0134).""" parts: list[object] = [ diff --git a/messagefoundry_webconsole/routes/admin.py b/messagefoundry_webconsole/routes/admin.py index e0b941b49..817c6ff06 100644 --- a/messagefoundry_webconsole/routes/admin.py +++ b/messagefoundry_webconsole/routes/admin.py @@ -22,6 +22,7 @@ UserUpdateRequest, ) from messagefoundry.auth import Identity, Permission +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.permissions import CUSTOM_ROLE_FORBIDDEN_PERMISSIONS from messagefoundry.auth.service import ( STEP_UP_ACTION_ADMIN_RESET_MFA, @@ -235,10 +236,10 @@ async def ui_user_channel_scope( assert_same_origin(request) form = dict(await _form_pairs(request)) names = [ln.strip() for ln in form.get("channels", "").splitlines() if ln.strip()] - # The tri-state scope_mode keeps deny-all ([]) distinguishable from all-channels (None) — - # an empty textarea alone must never widen a stored deny-all scope (review PR2-M3). - # Absent (a pre-tri-state cached form) defaults to "list"; any OTHER value is a - # hand-crafted post — refused rather than guessed (deny-by-default). + # The tri-state scope_mode keeps deny-all distinguishable from all-channels — an empty + # textarea alone must never widen a stored deny-all scope (review PR2-M3). Absent (a + # pre-tri-state cached form) defaults to "list"; any OTHER value is a hand-crafted post — + # refused rather than guessed (deny-by-default). mode = form.get("scope_mode", "list") if mode not in ("all", "list", "none"): return await _user_detail( @@ -255,7 +256,9 @@ async def ui_user_channel_scope( ), status_code=400, ) - channels = None if mode == "all" else ([] if mode == "none" else names) + # BACKLOG #1152: all-channels is now the explicit ALL_CHANNELS grant, not a null scope. Null + # and [] both deny, so posting null for "all" would have silently inverted this form. + channels = [ALL_CHANNELS] if mode == "all" else ([] if mode == "none" else names) try: body = ChannelScope(channels=channels) await admin.set_channel_scope(user_id, body=body, service=service, identity=identity) diff --git a/messagefoundry_webconsole/routes/core.py b/messagefoundry_webconsole/routes/core.py index 73bd7c8f4..75da3b3cd 100644 --- a/messagefoundry_webconsole/routes/core.py +++ b/messagefoundry_webconsole/routes/core.py @@ -360,7 +360,10 @@ async def ui_dashboard( identity: Identity = Depends(require_ui(Permission.MONITORING_READ)), ) -> HTMLResponse: rows = await core.list_connections(engine=engine, identity=identity) - return HTMLResponse(pages.dashboard(rows)) + # BACKLOG #1152: the landing page is where a fresh operator forms the impression that RBAC + # is broken, so it is where the unprovisioned state gets a sentence. Read off the identity, + # never off `not rows` — an estate with no connections configured yet is a different empty. + return HTMLResponse(pages.dashboard(rows, unprovisioned=identity.has_no_channels)) @app.get("/ui/connections", response_class=HTMLResponse) async def ui_connections( diff --git a/messagefoundry_webconsole/routes/uploaded_logs.py b/messagefoundry_webconsole/routes/uploaded_logs.py index 7f852415c..894fc6db9 100644 --- a/messagefoundry_webconsole/routes/uploaded_logs.py +++ b/messagefoundry_webconsole/routes/uploaded_logs.py @@ -143,17 +143,57 @@ def _refused(code: str) -> RedirectResponse: return RedirectResponse(f"{_FAILED_TARGET}?e={code}", status_code=303) +#: Page size the confirm pages walk the listing in. They need ONE file's metadata, not a page, so +#: they scan every page rather than the first — see :func:`_visible_file`. The engine's own ceiling. +_SCAN_PAGE = 500 + + def register(app: FastAPI, deps: UiDeps) -> None: core = deps.core + async def _visible_file( + request: Request, *, engine: Any, identity: Identity, file_id: str + ) -> Any: + """One visible file's metadata by id, or ``None`` — walked across the PAGED listing. + + The confirm pages read metadata through the listing rather than by id on purpose: the listing + is owner-scoped, so a file the caller may not see is simply absent and the page 303s instead + of disclosing that it exists. BACKLOG #1152 paged that listing, which silently broke the + shape — a first-page-only scan would have redirected an operator away from their OWN file the + moment they had more than a page of uploads, and reported it as "not found". + + Called with every parameter spelled out. These handlers are invoked BY REFERENCE across the + seam, never over HTTP, so a FastAPI ``Query(...)`` default left unfilled arrives as a Query + OBJECT rather than an int — which is exactly how this broke first: the object reached a list + slice and raised TypeError inside the route. + """ + offset = 0 + while True: + data = await core.list_uploaded_files( + request, engine=engine, identity=identity, limit=_SCAN_PAGE, offset=offset + ) + match = next((f for f in data.files if f.file_id == file_id), None) + if match is not None: + return match + offset += _SCAN_PAGE + if offset >= data.total or not data.files: + return None + @app.get("/ui/uploaded-logs", response_class=HTMLResponse) async def ui_uploaded_logs( request: Request, engine: Any = Depends(deps.get_engine), identity: Identity = Depends(require_ui(Permission.FILES_BROWSE)), e: str | None = Query(None, max_length=32), + # BACKLOG #1152: the listing is paged. Declared with the SAME bounds as the JSON route, so a + # hand-typed /ui query answers 422 here rather than reaching the handler with a value the + # engine would clamp differently -- the console must not be the looser of the two doors. + limit: int = Query(50, ge=1, le=500), + offset: int = Query(0, ge=0), ) -> HTMLResponse: - data = await core.list_uploaded_files(request, engine=engine, identity=identity) + data = await core.list_uploaded_files( + request, engine=engine, identity=identity, limit=limit, offset=offset + ) # The refused-mutation banner. An EXACT-key lookup in the allow-list, so the rendered string is # always one this module wrote — `e` itself is never rendered, echoed, or passed on, and an # unrecognized value yields no banner rather than reflected text. @@ -432,8 +472,7 @@ async def ui_uploaded_log_resend_confirm( # # Metadata read via list, exactly as delete-confirm does — a bad or absent id 404s at the # browse handler, and this route must not disclose one file's existence to a non-owner. - data = await core.list_uploaded_files(request, engine=engine, identity=identity) - match = next((f for f in data.files if f.file_id == file_id), None) + match = await _visible_file(request, engine=engine, identity=identity, file_id=file_id) if match is None: return RedirectResponse("/ui/uploaded-logs", status_code=303) return HTMLResponse(pages.uploaded_log_resend_confirm(file_id, match.filename, index, to)) @@ -447,8 +486,7 @@ async def ui_uploaded_log_delete_confirm( ) -> Response: # The confirm step (BACKLOG #126). Show the filename so the operator confirms the right file; a # bad/absent id (path-traversal) 404s at the browse handler, so read metadata via list here. - data = await core.list_uploaded_files(request, engine=engine, identity=identity) - match = next((f for f in data.files if f.file_id == file_id), None) + match = await _visible_file(request, engine=engine, identity=identity, file_id=file_id) if match is None: return RedirectResponse("/ui/uploaded-logs", status_code=303) return HTMLResponse(pages.uploaded_log_delete_confirm(file_id, match.filename)) diff --git a/packaging/messagefoundry-webconsole/tests/test_uploaded_logs_ui.py b/packaging/messagefoundry-webconsole/tests/test_uploaded_logs_ui.py index 327ed4d36..5cbeb2188 100644 --- a/packaging/messagefoundry-webconsole/tests/test_uploaded_logs_ui.py +++ b/packaging/messagefoundry-webconsole/tests/test_uploaded_logs_ui.py @@ -15,6 +15,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.models import ConnectorType from messagefoundry.config.settings import AuthSettings, StoreSettings @@ -36,8 +37,13 @@ async def _add_user( service: AuthService, name: str, role: Role, *, channels: list[str] | None = None ) -> str: - """Create a sign-in-ready local user. ``channels`` sets the per-channel RBAC scope (None = all); - it is applied BEFORE the first login because set_channel_scope revokes the user's sessions.""" + """Create a sign-in-ready local user. ``channels`` sets the per-channel RBAC scope; it is applied + BEFORE the first login because set_channel_scope revokes the user's sessions. + + ``None`` means "this test is not about the channel axis" and grants the whole estate. Before + BACKLOG #1152 that was what saying nothing already did; an unset scope now denies, and leaving + these fixtures unscoped would answer 403 on the resend target inbound for a reason this file -- + which is about the uploads OWNER axis (ASVS 8.2.2) -- never asserts.""" uid = await service.create_local_user( username=name, password=PW, display_name=None, email=None, roles=[role.value], actor="t" ) @@ -46,8 +52,10 @@ async def _add_user( await service.store.set_password( uid, password_hash=user.password_hash, must_change_password=False ) - if channels is not None: - await service.set_channel_scope(uid, channels, actor="t") + # `is None`, not falsiness: an explicitly EMPTY list means deny-all and must survive as one. + await service.set_channel_scope( + uid, [ALL_CHANNELS] if channels is None else channels, actor="t" + ) return uid @@ -137,6 +145,73 @@ async def test_uploaded_logs_ui_flow(engine: Engine, tmp_path: Path) -> None: assert "acme.hl7" not in (await c.get("/ui/uploaded-logs")).text +async def test_uploaded_logs_list_is_paged(engine: Engine, tmp_path: Path) -> None: + """BACKLOG #1152: the console listing pages, and its counter says window-of-total. + + A bare count could not distinguish "you have three files" from "you are looking at three of + forty", which is the reading that makes a pager invisible.""" + service = await _service(engine, ("op", Role.OPERATOR)) + transport = httpx.ASGITransport(app=_app(engine, service, tmp_path)) + async with httpx.AsyncClient(transport=transport, base_url="http://t") as c: + await _login(c, "op") + for n in range(3): + up = await c.post( + "/ui/uploaded-logs/upload", + files={"file": (f"page{n}.hl7", BATCH, "application/octet-stream")}, + ) + assert up.status_code in (200, 303), up.text + + first = await c.get("/ui/uploaded-logs", params={"limit": 2, "offset": 0}) + assert first.status_code == 200 + assert "1-2 of 3 file(s)" in first.text + assert "Next" in first.text and "Previous" not in first.text + assert "/ui/uploaded-logs?limit=2&offset=2" in first.text + + last = await c.get("/ui/uploaded-logs", params={"limit": 2, "offset": 2}) + assert "3-3 of 3 file(s)" in last.text + assert "Previous" in last.text and "Next" not in last.text + # The two pages together are the whole set, and neither shows the other's file. + shown = [n for n in range(3) if f"page{n}.hl7" in first.text + last.text] + assert shown == [0, 1, 2] + assert sum(f"page{n}.hl7" in first.text for n in range(3)) == 2 + + # The /ui door is no looser than the JSON one: out-of-range bounds are refused, not clamped. + assert (await c.get("/ui/uploaded-logs", params={"limit": 501})).status_code == 422 + assert (await c.get("/ui/uploaded-logs", params={"offset": -1})).status_code == 422 + + +async def test_confirm_pages_find_a_file_beyond_the_first_page( + engine: Engine, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """BACKLOG #1152 regression: paging the listing must not hide a file from its OWN confirm page. + + The delete/resend confirm pages read one file's metadata THROUGH the owner-scoped listing, so a + file the caller may not see is simply absent and the page 303s rather than disclosing it. Paging + that listing silently broke the shape: a first-page-only scan would redirect an operator away + from their own file the moment they had more than a page of uploads, and report it as not found + — a denial indistinguishable from the real one. + + The scan page size is monkeypatched down rather than uploading 500 files, which would make this + test minutes long to assert something that is about the LOOP, not about the number.""" + from messagefoundry_webconsole.routes import uploaded_logs as ul_routes + + monkeypatch.setattr(ul_routes, "_SCAN_PAGE", 1) + service = await _service(engine, ("op", Role.OPERATOR)) + transport = httpx.ASGITransport(app=_app(engine, service, tmp_path)) + async with httpx.AsyncClient(transport=transport, base_url="http://t") as c: + await _login(c, "op") + ids = [await _upload(c, f"deep{n}.hl7") for n in range(3)] + # The OLDEST upload sorts last (newest first), so it is off page one by construction. + oldest = ids[0] + confirm = await c.get(f"/ui/uploaded-logs/file/{oldest}/delete-confirm") + assert confirm.status_code == 200, confirm.text + assert "deep0.hl7" in confirm.text + + # An id nobody uploaded still 303s away rather than 200-ing on an empty page. + missing = await c.get(f"/ui/uploaded-logs/file/{'0' * 32}/delete-confirm") + assert missing.status_code == 303 + + def test_upload_form_states_consent_affordance() -> None: # ASVS 14.2.8: the upload form states, above the submit button, what non-body metadata is retained and # who sees it — submitting the form IS the consent (no separate stored flag). Pure page-render check. diff --git a/packaging/messagefoundry-webconsole/tests/test_webui.py b/packaging/messagefoundry-webconsole/tests/test_webui.py index f0574d8b5..ef8e89574 100644 --- a/packaging/messagefoundry-webconsole/tests/test_webui.py +++ b/packaging/messagefoundry-webconsole/tests/test_webui.py @@ -19,7 +19,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Role -from messagefoundry.auth.identity import AuthProvider +from messagefoundry.auth.identity import ALL_CHANNELS, AuthProvider from messagefoundry.auth.service import AuthService from messagefoundry.auth.tokens import hash_token from messagefoundry.config.models import ConnectorType @@ -32,6 +32,7 @@ Send, ) from messagefoundry.pipeline import Engine +from messagefoundry_webconsole.pages.connections import UNPROVISIONED_NOTICE # The repo root is three levels up from this file (packaging/messagefoundry-webconsole/tests/) — the # suite moved out of the engine's tests/ into the package's own suite (Option B Phase 2, ADR 0065). @@ -70,6 +71,10 @@ async def _add(service: AuthService, username: str, *roles: Role) -> None: roles=[r.value for r in roles], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password( @@ -180,6 +185,43 @@ async def test_login_sets_confined_cookie_and_logout_revokes(engine: Engine) -> assert (await c.get("/ui")).status_code == 303 # → /ui/login +async def test_unprovisioned_operator_is_told_why_the_console_is_empty(engine: Engine) -> None: + """BACKLOG #1152: the first-run affordance for the deny-by-default channel scope. + + A fresh non-administrator is granted no channel, so every list is legitimately empty for them. + The landing page must SAY so — an unexplained empty console reads as broken RBAC on day one, + and the reflex fix for that symptom is to widen somebody's grant. It is deliberately a page + banner and not a start-time refusal, which would make a fresh single-operator install + unbootable for the same condition: asserted here by the page answering 200.""" + service = await _service(engine) + uid = await service.create_local_user( + username="fresh", + password=PW, + display_name=None, + email=None, + roles=[Role.OPERATOR.value], + actor="test", + ) + user = await service.store.get_user(uid) + assert user is not None and user.password_hash is not None + assert user.channel_scope is None # nobody has granted anything: the shipped create path + await service.store.set_password( + uid, password_hash=user.password_hash, must_change_password=False + ) + async with _client(engine, service) as c: + await _cookie_login(c, "fresh") + r = await c.get("/ui") + assert r.status_code == 200 # the console still loads; it explains itself + assert UNPROVISIONED_NOTICE in r.text + # Grant a channel and the notice goes away — the banner tracks the scope, not the emptiness + # of the estate (this engine has no connections configured either way). + await service.set_channel_scope(uid, ["IB_A"], actor="admin") + await _cookie_login(c, "fresh") # the scope change revoked the session + granted = await c.get("/ui") + assert granted.status_code == 200 + assert UNPROVISIONED_NOTICE not in granted.text + + async def test_bad_credentials_redirect_without_cookie(engine: Engine) -> None: service = await _service(engine) await _add(service, "op", Role.OPERATOR) @@ -2236,6 +2278,34 @@ async def test_users_page_lists_accounts(engine: Engine) -> None: assert 'href="/ui/users"' in (await c.get("/ui")).text +def test_users_page_scope_column_shows_what_the_account_reaches() -> None: + """BACKLOG #1152: the Channel scope column resolves BOTH inputs, not just the stored one. + + A stored ``None`` denies now, so it must not read "all". But an ADMINISTRATOR is all-channels by + role whatever is stored, and printing "(none)" beside an account that holds the whole estate + hides real access from the person whose job is to review it — the worse of the two errors.""" + from messagefoundry.api.auth_models import UserSummary + from messagefoundry_webconsole.pages.admin import _scope_cell + + def _u(roles: list[str], scope: list[str] | None) -> UserSummary: + return UserSummary( + id="u", + username="u", + auth_provider="local", + disabled=False, + roles=roles, + channel_scope=scope, + ) + + assert _scope_cell(_u(["operator"], None)) == "(none)" # never granted -> denies + assert _scope_cell(_u(["operator"], [])) == "(none)" # explicitly denied + assert _scope_cell(_u(["operator"], [ALL_CHANNELS])) == "all" # the typed grant + assert _scope_cell(_u(["operator"], ["IB_A", "IB_B"])) == "IB_A, IB_B" + # By role, and the stored scope is irrelevant in BOTH directions. + assert _scope_cell(_u(["administrator"], None)) == "all (administrator)" + assert _scope_cell(_u(["administrator"], ["IB_A"])) == "all (administrator)" + + async def test_l4a_actions_registered_in_correct_allowlists(engine: Engine) -> None: # Form pages are unlock targets; body-less path actions are auto-retry; body-carrying POST paths # are in NEITHER list (they can only be reached by a fresh same-origin form submit). @@ -2465,7 +2535,10 @@ async def test_channel_scope_roundtrip(engine: Engine) -> None: ) assert r.status_code == 303 user = await service.store.get_user(uid) - assert user is not None and user.channel_scope is None + # BACKLOG #1152: all-channels is the stored ["*"] grant, not a null scope — null now denies, + # so posting it for the "all" mode would have silently inverted this form. + assert user is not None and json_.loads(user.channel_scope or "null") == [ALL_CHANNELS] + assert 'value="all" selected' in (await c.get(f"/ui/users/{uid}")).text # "Only these" with an empty list is ambiguous — refused, scope unchanged. r = await c.post( f"/ui/users/{uid}/channel-scope", @@ -2474,7 +2547,7 @@ async def test_channel_scope_roundtrip(engine: Engine) -> None: ) assert r.status_code == 400 and "list at least one connection" in r.text user = await service.store.get_user(uid) - assert user is not None and user.channel_scope is None + assert user is not None and json_.loads(user.channel_scope or "null") == [ALL_CHANNELS] async def test_reset_password_shows_temp_once(engine: Engine) -> None: @@ -2660,6 +2733,10 @@ async def _add_with_role_ids(service: AuthService, username: str, role_ids: list roles=role_ids, actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/golden/webconsole_seam.snapshot b/tests/golden/webconsole_seam.snapshot index ecad57e77..85dedb576 100644 --- a/tests/golden/webconsole_seam.snapshot +++ b/tests/golden/webconsole_seam.snapshot @@ -7,7 +7,7 @@ # by hand (BACKLOG #1220) - so a newly rendered DTO is covered with nobody editing a list. ## ENGINE_UI_SEAM -266cbfd342b22819 +767521d4399d1ef4 ## dataclass messagefoundry.api._ui_seam.UiDeps engine_seam @@ -222,7 +222,7 @@ messagefoundry.api.models.SystemStatus: claim_proc, db, engine, kpis, logs, pool messagefoundry.api.models.UpdateInfo: current_version, pinned_version, update_available messagefoundry.api.models.UploadResendRequest: index, to messagefoundry.api.models.UploadedFileInfo: content_type, file_id, filename, message_count, sha256, size, uploaded_at, uploader -messagefoundry.api.models.UploadedFileList: files, scope, total +messagefoundry.api.models.UploadedFileList: files, limit, offset, scope, total messagefoundry.api.models.UploadedMessageSearchRequest: content, control_id, field_path, field_value, limit, message_type, offset, target messagefoundry.api.models.UploadedMessageSummary: control_id, index, message_type, size messagefoundry.api.models.UploadedMessagesResult: file_id, filename, matched, messages, scanned, total_messages, truncated diff --git a/tests/test_ad_group_scope.py b/tests/test_ad_group_scope.py index 9bc7180b6..b8b93c61d 100644 --- a/tests/test_ad_group_scope.py +++ b/tests/test_ad_group_scope.py @@ -13,7 +13,7 @@ from messagefoundry.api import create_app from messagefoundry.auth.permissions import Role -from messagefoundry.auth.service import AuthService +from messagefoundry.auth.service import AuthService, _allowed_channels from messagefoundry.config.settings import AuthSettings from messagefoundry.pipeline import Engine from messagefoundry.store.store import MessageStore @@ -75,7 +75,11 @@ async def test_sync_star_means_all_and_admin_is_untouched(tmp_path: Path) -> Non await store.set_user_channel_scope("eve", json.dumps(["IB_A"])) # previously scoped scoped = await store.get_user("eve") refreshed = await service._sync_ad_channel_scope(scoped, frozenset(), ["grp-all"]) - assert refreshed.channel_scope is None # '*' clears to all channels + # BACKLOG #1152: '*' persists the EXPLICIT all-channels grant. It used to persist SQL NULL + # and lean on NULL meaning "all"; NULL now denies, so that collapse would have inverted a + # deliberate wildcard mapping into a deny-everything one. + assert json.loads(refreshed.channel_scope) == ["*"] + assert _allowed_channels(refreshed, frozenset()) is None # and it resolves to all channels admin = await _ad_user(store, "boss") out = await service._sync_ad_channel_scope( diff --git a/tests/test_admin_new_ip.py b/tests/test_admin_new_ip.py index e4874a041..58339ae71 100644 --- a/tests/test_admin_new_ip.py +++ b/tests/test_admin_new_ip.py @@ -21,7 +21,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Role, totp -from messagefoundry.auth.identity import Identity +from messagefoundry.auth.identity import ALL_CHANNELS, Identity from messagefoundry.auth.notifications import ADMIN_NEW_IP, SecurityEvent from messagefoundry.auth.service import AuthService from messagefoundry.auth.tokens import hash_token @@ -53,6 +53,10 @@ async def _enabled_admin(service: AuthService, *, client: str) -> tuple[str, Ide roles=[Role.ADMINISTRATOR.value], actor="t", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") user = await service.store.get_user(uid) assert user is not None and user.password_hash is not None await service.store.set_password( @@ -114,6 +118,10 @@ async def test_missing_baseline_and_bad_tokens_not_flagged() -> None: roles=[Role.VIEWER.value], actor="t", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") # A session with no recorded login address is never penalized (avoids spurious friction). await store.create_session( token_hash=hash_token("noip"), user_id=uid, expires_at=2e12, client=None @@ -185,6 +193,10 @@ async def test_loopback_addresses_treated_as_same_host() -> None: roles=[Role.ADMINISTRATOR.value], actor="t", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") await store.create_session( token_hash=hash_token("lb"), user_id=uid, expires_at=2e12, client="::1" ) @@ -257,6 +269,10 @@ async def _add_admin(service: AuthService, username: str) -> None: roles=[Role.ADMINISTRATOR.value], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password( @@ -335,6 +351,10 @@ async def test_new_ip_never_overrides_rbac(engine: Engine) -> None: roles=[Role.VIEWER.value], actor="t", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") user = await service.store.get_user(uid) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_ai_broker.py b/tests/test_ai_broker.py index 7bd1c257a..db0d501c8 100644 --- a/tests/test_ai_broker.py +++ b/tests/test_ai_broker.py @@ -20,6 +20,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.ai_policy import AiDataScope, AiMode, resolve_effective_policy from messagefoundry.config.settings import AiSettings, AuthSettings @@ -302,6 +303,10 @@ async def test_ai_chat_requires_ai_assist_permission( roles=[role.value], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None # Admin-created accounts force first-login rotation (WP-L3-12); clear it for a usable login so diff --git a/tests/test_alerts_rules_api.py b/tests/test_alerts_rules_api.py index 7bed7993a..34a64ef89 100644 --- a/tests/test_alerts_rules_api.py +++ b/tests/test_alerts_rules_api.py @@ -23,6 +23,7 @@ from messagefoundry.api import create_app, create_managed_app from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.settings import ( AlertRule, @@ -189,6 +190,10 @@ async def _add(service: AuthService, username: str, *roles: Role) -> None: roles=[r.value for r in roles], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_alerts_test_email.py b/tests/test_alerts_test_email.py index 212b8aa16..51a1e3745 100644 --- a/tests/test_alerts_test_email.py +++ b/tests/test_alerts_test_email.py @@ -26,6 +26,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.settings import AlertsSettings, AuthSettings from messagefoundry.pipeline import Engine @@ -232,6 +233,10 @@ async def _add(service: AuthService, username: str, *roles: Role) -> None: roles=[r.value for r in roles], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_api_alerts.py b/tests/test_api_alerts.py index cd2eb3ccf..613c11b70 100644 --- a/tests/test_api_alerts.py +++ b/tests/test_api_alerts.py @@ -13,6 +13,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.models import ConnectorType from messagefoundry.config.settings import AuthSettings @@ -199,6 +200,10 @@ async def _add(service: AuthService, username: str, *roles: Role) -> str: roles=[r.value for r in roles], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_api_auth.py b/tests/test_api_auth.py index 4c67231aa..769408bb0 100644 --- a/tests/test_api_auth.py +++ b/tests/test_api_auth.py @@ -13,6 +13,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Role, totp +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.ldap import AdPrincipal from messagefoundry.auth.service import AuthService from messagefoundry.auth.tokens import hash_token @@ -70,6 +71,11 @@ async def _clear_must_change(service: AuthService, user_id: str) -> None: await service.store.set_password( user_id, password_hash=user.password_hash, must_change_password=False ) + # BACKLOG #1152: an unset channel scope now denies. This file gates on ROLES and PERMISSIONS and + # asserts nothing about the channel axis (grep: zero references to channel_scope / + # can_access_channel), so grant the estate explicitly rather than let a second, unrelated control + # decide these outcomes. The channel axis is covered in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") async def _login( @@ -1155,6 +1161,10 @@ def resolve_principal(self, username: str) -> AdPrincipal | None: service = AuthService(engine.store, settings, ldap=_FakeLdap()) # type: ignore[arg-type] await service.initialize() await service.set_ad_group_map([("CN=MF-Ops,DC=x", "operator")], actor="admin") + # BACKLOG #1152: an AD principal with no group-to-channel mapping is scoped to nothing, so the + # connections:control assertion below would answer 403 for a reason this test is not about. Map + # the same group to the wildcard channel — the AD-native way to say "the whole estate". + await engine.store.set_ad_group_scope_map([("CN=MF-Ops,DC=x", ALL_CHANNELS)]) async with _client(engine, service) as c: # AD PASSWORD LOGIN is retired (BACKLOG #1137), so the session is minted through the tail # Kerberos and OIDC both end at. The subject is unchanged: an AD-provider session carries the diff --git a/tests/test_approvals.py b/tests/test_approvals.py index 881ed5494..a5eab7c20 100644 --- a/tests/test_approvals.py +++ b/tests/test_approvals.py @@ -20,6 +20,7 @@ from messagefoundry.api import create_app from messagefoundry.api.approvals import ApprovalGate from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.models import ConnectorType from messagefoundry.config.settings import ApprovalsSettings, AuthSettings @@ -70,6 +71,10 @@ async def _add(service: AuthService, username: str, *roles: Role) -> None: roles=[r.value for r in roles], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") user = await service.store.get_user(uid) # clear forced first-login rotation (WP-L3-12) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_attachment_download_api.py b/tests/test_attachment_download_api.py index c89aa82e0..aa0470a47 100644 --- a/tests/test_attachment_download_api.py +++ b/tests/test_attachment_download_api.py @@ -33,6 +33,7 @@ from messagefoundry.api import create_app from messagefoundry.api.app import _ATTACHMENT_CSP from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.settings import AuthSettings from messagefoundry.pipeline import Engine @@ -344,6 +345,10 @@ async def test_ui_delegate_serves_the_sandbox_csp_not_the_console_csp( roles=[Role.OPERATOR.value], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") user = await service.store.get_user(uid) assert user is not None and user.password_hash is not None await service.store.set_password( @@ -405,6 +410,10 @@ async def _add(service: AuthService, username: str, *roles: Role) -> str: roles=[r.value for r in roles], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") user = await service.store.get_user(uid) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_auth_hardening.py b/tests/test_auth_hardening.py index 703bbc3ab..8bb17e721 100644 --- a/tests/test_auth_hardening.py +++ b/tests/test_auth_hardening.py @@ -29,6 +29,7 @@ from messagefoundry.api import create_app from messagefoundry.api.app import _emit_bootstrap_admin, _session_reaper from messagefoundry.auth import Role, hash_password +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.ldap import AdPrincipal, LdapAuthenticator, LdapError from messagefoundry.auth.service import AuthService, BootstrapAdmin from messagefoundry.config.settings import AuthSettings, StoreSettings @@ -71,6 +72,10 @@ async def _add(service: AuthService, username: str, *roles: Role) -> None: roles=[r.value for r in roles], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") # Admin-created accounts force first-login rotation (WP-L3-12); clear it so these fixtures behave # like already-onboarded users (keeping the same hash). user = await service.store.get_user(user_id) diff --git a/tests/test_channel_rbac.py b/tests/test_channel_rbac.py index ba745f1d8..3976cdb07 100644 --- a/tests/test_channel_rbac.py +++ b/tests/test_channel_rbac.py @@ -12,6 +12,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import AuthProvider, Identity, Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.models import ConnectorType from messagefoundry.config.settings import AuthSettings @@ -79,8 +80,21 @@ async def _login(c: httpx.AsyncClient, username: str) -> dict[str, str]: def test_identity_can_access_channel() -> None: - allc = Identity.build(user_id="1", username="u", auth_provider=AuthProvider.LOCAL, roles=[]) + # BACKLOG #1152: omitting allowed_channels DENIES. The wide scope is now spelled out. + unprovisioned = Identity.build( + user_id="0", username="fresh", auth_provider=AuthProvider.LOCAL, roles=[] + ) + assert not unprovisioned.can_access_channel("anything") + assert unprovisioned.has_no_channels + allc = Identity.build( + user_id="1", + username="u", + auth_provider=AuthProvider.LOCAL, + roles=[], + allowed_channels=None, + ) assert allc.can_access_channel("anything") # None scope = all + assert not allc.has_no_channels scoped = Identity.build( user_id="2", username="s", @@ -211,7 +225,10 @@ async def test_scoped_user_graph_edges_hides_shared_outbound(engine: Engine) -> service = await _service(engine) scoped_uid = await _add(service, "op", Role.OPERATOR) await service.set_channel_scope(scoped_uid, ["IB_A"], actor="admin") - await _add(service, "boss", Role.OPERATOR) # NO channel scope → unscoped + wide_uid = await _add(service, "boss", Role.OPERATOR) + # An explicit all-channels grant (BACKLOG #1152). Leaving the scope unset used to mean this and + # now denies, so the estate-wide arm of the comparison has to be granted rather than assumed. + await service.set_channel_scope(wide_uid, [ALL_CHANNELS], actor="admin") async with _client(engine, service) as c: scoped = (await c.get("/graph/edges", headers=await _login(c, "op"))).json() @@ -293,16 +310,44 @@ async def test_credential_test_route_authorizes_before_disclosing_config(engine: assert (await c.post("/connections/IB_NOPE/test-credential", headers=h)).status_code == 404 -async def test_unscoped_user_and_admin_have_full_access(engine: Engine) -> None: +async def test_unprovisioned_operator_reaches_nothing_until_granted(engine: Engine) -> None: + """BACKLOG #1152 (ASVS 8.2.2), the INVERSION of the test that used to pin the permissive default. + + A freshly minted non-administrator has never had a scope written -- ``create_user``'s INSERT does + not list ``channel_scope``, so the column is SQL NULL. That used to resolve to "every channel", + which meant every per-channel check in the API narrowed nobody on a default install. It now + resolves to the empty set, so the same account reaches no message until somebody grants a + channel, and the grant is what changes the answer -- asserted here in the same test so a + regression that re-widens the default cannot pass by leaving both arms denied.""" service = await _service(engine) - await _add(service, "op", Role.OPERATOR) # no scope set → NULL → all channels + op_id = await _add(service, "op", Role.OPERATOR) admin_id = await _add(service, "boss", Role.ADMINISTRATOR) await service.set_channel_scope(admin_id, ["IB_A"], actor="admin") # ignored for admins mid_b = await engine.store.enqueue_message(channel_id="IB_B", raw=ADT, deliveries=[("d", ADT)]) async with _client(engine, service) as c: - for who in ("op", "boss"): - h = await _login(c, who) - assert (await c.get(f"/messages/{mid_b}", headers=h)).status_code == 200 + # The unprovisioned operator: 404, the same answer another tenant's message gets. + assert (await c.get(f"/messages/{mid_b}", headers=await _login(c, "op"))).status_code == 404 + # The administrator is all-channels by role, so the first operator of a fresh install is + # never locked out of their own console. + assert ( + await c.get(f"/messages/{mid_b}", headers=await _login(c, "boss")) + ).status_code == 200 + # Grant the channel and the SAME account now reaches it. + await service.set_channel_scope(op_id, ["IB_B"], actor="admin") + assert (await c.get(f"/messages/{mid_b}", headers=await _login(c, "op"))).status_code == 200 + + +async def test_explicit_all_channels_grant_reaches_the_estate(engine: Engine) -> None: + """BACKLOG #1152: all-channels survives as a grant somebody typed -- the ``*`` token in the + stored scope -- so an operator who genuinely needs the estate can still be given it without the + ADMINISTRATOR role. This is the arm that keeps the default flip from being a capability loss.""" + service = await _service(engine) + op_id = await _add(service, "op", Role.OPERATOR) + mid_b = await engine.store.enqueue_message(channel_id="IB_B", raw=ADT, deliveries=[("d", ADT)]) + async with _client(engine, service) as c: + assert (await c.get(f"/messages/{mid_b}", headers=await _login(c, "op"))).status_code == 404 + await service.set_channel_scope(op_id, [ALL_CHANNELS], actor="admin") + assert (await c.get(f"/messages/{mid_b}", headers=await _login(c, "op"))).status_code == 200 async def test_channel_scope_admin_endpoint_roundtrip(engine: Engine) -> None: diff --git a/tests/test_client_network_allowlist.py b/tests/test_client_network_allowlist.py index 8cd47d63b..9f69a8599 100644 --- a/tests/test_client_network_allowlist.py +++ b/tests/test_client_network_allowlist.py @@ -29,6 +29,7 @@ from messagefoundry.api import create_app from messagefoundry.api.client_networks import DENIAL_HEADER, DENIAL_MARKER, ClientNetworkMiddleware from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.settings import ( AlertsSettings, @@ -80,6 +81,10 @@ async def _add_viewer(service: AuthService, username: str) -> None: roles=[Role.VIEWER.value], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_connection_event_scope.py b/tests/test_connection_event_scope.py index 5e5ea0abd..1eb017dbf 100644 --- a/tests/test_connection_event_scope.py +++ b/tests/test_connection_event_scope.py @@ -17,6 +17,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.models import ConnectorType from messagefoundry.config.settings import AuthSettings @@ -241,9 +242,12 @@ async def test_event_info_carries_no_phi_field(engine: Engine) -> None: assert closed["reason"] == "eof" -async def test_unscoped_operator_sees_full_estate(engine: Engine) -> None: +async def test_all_channels_operator_sees_full_estate(engine: Engine) -> None: service = await _service(engine) - await _add(service, "op", Role.OPERATOR) # no scope → NULL → all channels + uid = await _add(service, "op", Role.OPERATOR) + # BACKLOG #1152: an unset scope now denies, so the estate-wide operator this test is about is + # created by an EXPLICIT '*' grant rather than by saying nothing. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="admin") async with _client(engine, service) as c: h = await _login(c, "op") chans = (await c.get("/channels", headers=h)).json() diff --git a/tests/test_content_search.py b/tests/test_content_search.py index 3c28beffa..f62126bd4 100644 --- a/tests/test_content_search.py +++ b/tests/test_content_search.py @@ -22,6 +22,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.auth.tokens import hash_token from messagefoundry.config.settings import AuthSettings @@ -269,6 +270,10 @@ async def _add_user(service: AuthService, username: str, roles: list[str]) -> No user_id = await service.create_local_user( username=username, password=PW, display_name=None, email=None, roles=roles, actor="test" ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_custom_roles.py b/tests/test_custom_roles.py index f7de5b240..7eb28392d 100644 --- a/tests/test_custom_roles.py +++ b/tests/test_custom_roles.py @@ -20,6 +20,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Identity, Permission, Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.permissions import ( CUSTOM_ROLE_ID_PREFIX, CustomRoleError, @@ -53,6 +54,10 @@ async def _make_user(service: AuthService, username: str, *role_ids: str) -> str roles=list(role_ids), actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") return user_id @@ -326,6 +331,10 @@ async def test_crud_requires_users_manage(engine: Engine) -> None: roles=[Role.VIEWER.value], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(viewer_id, [ALL_CHANNELS], actor="test") u = await engine.store.get_user(viewer_id) assert u is not None and u.password_hash is not None await engine.store.set_password( diff --git a/tests/test_dr_rbac.py b/tests/test_dr_rbac.py index e919cfe0f..8b8d5d597 100644 --- a/tests/test_dr_rbac.py +++ b/tests/test_dr_rbac.py @@ -16,6 +16,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Permission, Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.permissions import ( BUILTIN_ROLE_PERMISSIONS, CUSTOM_ROLE_FORBIDDEN_PERMISSIONS, @@ -87,6 +88,10 @@ async def _add(service: AuthService, username: str, *roles: Role) -> None: roles=[r.value for r in roles], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_field_authz_enforcement_sites.py b/tests/test_field_authz_enforcement_sites.py index 3988952ce..529dc64a8 100644 --- a/tests/test_field_authz_enforcement_sites.py +++ b/tests/test_field_authz_enforcement_sites.py @@ -33,6 +33,7 @@ from messagefoundry.api import create_app from messagefoundry.api.field_authz import PHI_FIELDS from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.permissions import Permission from messagefoundry.auth.service import AuthService from messagefoundry.config.settings import AuthSettings @@ -146,6 +147,10 @@ async def seeded(tmp_path: Path) -> AsyncIterator[_Seed]: roles=roles, actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") user = await service.store.get_user(uid) assert user is not None and user.password_hash is not None # Admin-created accounts force first-login rotation (WP-L3-12); clear it, keep the hash. diff --git a/tests/test_logging_surfaces.py b/tests/test_logging_surfaces.py index 3ef08a0b2..11d41e628 100644 --- a/tests/test_logging_surfaces.py +++ b/tests/test_logging_surfaces.py @@ -22,6 +22,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.settings import AuthSettings from messagefoundry.logging_setup import ( @@ -113,6 +114,10 @@ async def _add_user(service: AuthService, username: str, roles: list[str]) -> No user_id = await service.create_local_user( username=username, password=PW, display_name=None, email=None, roles=roles, actor="test" ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_message_export.py b/tests/test_message_export.py index 7a5399c6f..43d5b094c 100644 --- a/tests/test_message_export.py +++ b/tests/test_message_export.py @@ -20,6 +20,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Permission, Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.permissions import BUILTIN_ROLE_PERMISSIONS from messagefoundry.auth.service import AuthService from messagefoundry.auth.tokens import hash_token @@ -73,6 +74,10 @@ async def _add_user( user_id = await service.create_local_user( username=username, password=PW, display_name=None, email=None, roles=roles, actor="test" ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_metrics_exporter.py b/tests/test_metrics_exporter.py index 8c65b7882..6cbeb7623 100644 --- a/tests/test_metrics_exporter.py +++ b/tests/test_metrics_exporter.py @@ -30,6 +30,7 @@ render_metrics, ) from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.settings import AuthSettings from messagefoundry.pipeline import Engine @@ -301,6 +302,10 @@ async def _add(service: AuthService, username: str, *roles: Role) -> None: roles=[r.value for r in roles], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_mfa_access_gate.py b/tests/test_mfa_access_gate.py index bba15d82a..65e81b610 100644 --- a/tests/test_mfa_access_gate.py +++ b/tests/test_mfa_access_gate.py @@ -20,6 +20,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Role, totp +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.ldap import AdPrincipal from messagefoundry.auth.service import AuthService from messagefoundry.config.settings import AuthSettings @@ -55,6 +56,10 @@ async def _add(service: AuthService, username: str, *roles: Role) -> str: roles=[r.value for r in roles], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") # Admin-created accounts force first-login rotation; clear it so must_change does not mask the # MFA refusal under test (must_change is enforced FIRST by design). user = await service.store.get_user(user_id) diff --git a/tests/test_resend.py b/tests/test_resend.py index 806854d41..7ef0fc339 100644 --- a/tests/test_resend.py +++ b/tests/test_resend.py @@ -17,6 +17,7 @@ import pytest from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.models import ConnectorType, RetryPolicy from messagefoundry.config.settings import AuthSettings @@ -402,6 +403,10 @@ async def test_resend_requires_access_to_the_alternate_outbound_channel(tmp_path roles=[Role.OPERATOR.value], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") user = await service.store.get_user(uid) assert user is not None and user.password_hash is not None await service.store.set_password( @@ -447,6 +452,10 @@ async def test_resend_denied_without_the_resend_permission(tmp_path: Path) -> No roles=[Role.VIEWER.value], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") user = await service.store.get_user(uid) assert user is not None and user.password_hash is not None await service.store.set_password( @@ -494,6 +503,10 @@ async def test_resend_grant_is_audited_even_when_it_fails_downstream(tmp_path: P roles=[Role.OPERATOR.value], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") user = await service.store.get_user(uid) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_response_capture.py b/tests/test_response_capture.py index a6189a8da..cfbddb3ed 100644 --- a/tests/test_response_capture.py +++ b/tests/test_response_capture.py @@ -15,6 +15,7 @@ import pytest +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.config.models import ConnectorType, Destination from messagefoundry.config.response import activated, response_get from messagefoundry.config.run_context import ROUTER, TRANSFORM, RunContext, run_contexts @@ -606,6 +607,10 @@ async def test_responses_route_rbac_and_audit(tmp_path: Any) -> None: roles=[role.value], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") # Admin-created accounts force first-login rotation (WP-L3-12); clear it (keep the hash). u = await service.store.get_user(uid) assert u is not None and u.password_hash is not None diff --git a/tests/test_search_presets_api.py b/tests/test_search_presets_api.py index 204ff7ba0..b0d1cb054 100644 --- a/tests/test_search_presets_api.py +++ b/tests/test_search_presets_api.py @@ -13,6 +13,7 @@ import pytest from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.settings import AuthSettings from messagefoundry.pipeline import Engine @@ -36,6 +37,10 @@ async def _user(engine: Engine, role: Role, name: str) -> AuthService: uid = await service.create_local_user( username=name, password=PW, display_name=None, email=None, roles=[role.value], actor="t" ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") user = await service.store.get_user(uid) assert user is not None and user.password_hash is not None await service.store.set_password( @@ -267,6 +272,10 @@ async def test_a_recreated_username_does_not_inherit_the_departed_operators_pres roles=[Role.OPERATOR.value], actor="t", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(new_id, [ALL_CHANNELS], actor="test") fresh = await service.store.get_user(new_id) assert fresh is not None and fresh.password_hash is not None await service.store.set_password( diff --git a/tests/test_startup_fault_isolation.py b/tests/test_startup_fault_isolation.py index 2571aea97..7dc1b6279 100644 --- a/tests/test_startup_fault_isolation.py +++ b/tests/test_startup_fault_isolation.py @@ -16,6 +16,7 @@ import pytest +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.config.models import ConnectorType, RetryPolicy from messagefoundry.config.wiring import ( API_LISTENER_LABEL, @@ -424,6 +425,10 @@ async def test_connections_api_reports_degraded_outbound(tmp_path: Path) -> None roles=[Role.VIEWER.value], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") u = await service.store.get_user(uid) assert u is not None and u.password_hash is not None await service.store.set_password( diff --git a/tests/test_stats_reset.py b/tests/test_stats_reset.py index 5567f45f5..34b0b7901 100644 --- a/tests/test_stats_reset.py +++ b/tests/test_stats_reset.py @@ -18,6 +18,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.settings import AuthSettings from messagefoundry.pipeline import Engine @@ -173,6 +174,10 @@ async def _add(service: AuthService, username: str, *roles: Role) -> str: roles=[r.value for r in roles], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password( diff --git a/tests/test_step_up.py b/tests/test_step_up.py index 8c5e09f10..36dcd23c0 100644 --- a/tests/test_step_up.py +++ b/tests/test_step_up.py @@ -16,6 +16,7 @@ from messagefoundry.api import create_app from messagefoundry.auth import Role, totp +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.ldap import AdPrincipal from messagefoundry.auth.service import AuthService from messagefoundry.auth.tokens import hash_token @@ -56,6 +57,10 @@ async def _add_admin(service: AuthService, username: str) -> None: roles=[Role.ADMINISTRATOR.value], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None # Admin-created accounts force first-login rotation (WP-L3-12); clear it for a usable test login. @@ -271,6 +276,10 @@ async def test_admin_user_update_is_action_bound(engine: Engine) -> None: roles=[Role.VIEWER.value], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(target_id, [ALL_CHANNELS], actor="test") async with _client(engine, service) as c: token = await _login(c, "boss") body = {"display_name": "Renamed"} @@ -301,6 +310,10 @@ async def test_admin_user_update_opt_out_uses_window(engine: Engine) -> None: roles=[Role.VIEWER.value], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(target_id, [ALL_CHANNELS], actor="test") async with _client(engine, service) as c: token = await _login(c, "boss") r = await c.patch(f"/users/{target_id}", headers=_auth(token), json={"display_name": "X"}) @@ -434,6 +447,10 @@ async def test_create_session_stamps_reauth_at(engine: Engine) -> None: roles=[Role.VIEWER.value], actor="t", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") await engine.store.create_session( token_hash="deadbeef", user_id=uid, expires_at=2e12, client="t" ) diff --git a/tests/test_upload_api.py b/tests/test_upload_api.py index c40cc2b07..4fe21aa98 100644 --- a/tests/test_upload_api.py +++ b/tests/test_upload_api.py @@ -16,6 +16,7 @@ import pytest from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.permissions import ( BUILTIN_ROLE_PERMISSIONS, CUSTOM_ROLE_FORBIDDEN_PERMISSIONS, @@ -72,6 +73,11 @@ async def _make_user(engine: Engine, role: Role, *, name: str) -> AuthService: await service.store.set_password( uid, password_hash=user.password_hash, must_change_password=False ) + # BACKLOG #1152: an unset channel scope now denies. This file exercises the OWNER axis (ASVS + # 8.2.2 on uploaded files, which carry no channel), so grant the estate explicitly and leave the + # channel axis to tests/test_channel_rbac.py — otherwise upload RESEND, whose target inbound IS + # channel-checked, would fail here for a reason the file is not about. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") return service @@ -92,6 +98,11 @@ async def _add_user(service: AuthService, role: Role, *, name: str) -> str: await service.store.set_password( uid, password_hash=user.password_hash, must_change_password=False ) + # BACKLOG #1152: an unset channel scope now denies. This file exercises the OWNER axis (ASVS + # 8.2.2 on uploaded files, which carry no channel), so grant the estate explicitly and leave the + # channel axis to tests/test_channel_rbac.py — otherwise upload RESEND, whose target inbound IS + # channel-checked, would fail here for a reason the file is not about. + await service.set_channel_scope(uid, [ALL_CHANNELS], actor="test") return uid @@ -177,6 +188,60 @@ async def test_upload_list_browse_roundtrip(engine: Engine, tmp_path: Path) -> N assert "digits" in joined or "alnum" in joined # the needle SHAPE is +async def test_uploads_listing_is_paged(engine: Engine, tmp_path: Path) -> None: + """BACKLOG #1152: GET /uploads is paged, and ``total`` still counts the whole visible set. + + The route was pageless, so one response carried every file an install had ever accumulated. + Asserted here: the window is honoured, ``total`` is NOT the window, the pages partition the set + with no overlap and no gap, and a second request for the same window returns the same page -- + which is what the total sort order in ``UploadStore.list_files`` exists to guarantee, since these + five uploads land close enough together to tie on ``uploaded_at``.""" + pytest.importorskip("psutil") + from messagefoundry.api import create_app + + service = await _make_user(engine, Role.OPERATOR, name="op") + app = create_app(engine, auth=service, store_settings=_uploads_settings(tmp_path)) + transport = httpx.ASGITransport(app=app) + async with httpx.AsyncClient(transport=transport, base_url="http://t") as c: + h = await _login(c, "op") + for n in range(5): + up = await c.post( + "/uploads", files={"file": (f"f{n}.hl7", ADT, "text/plain")}, headers=h + ) + assert up.status_code == 200, up.text + + first = (await c.get("/uploads", params={"limit": 2, "offset": 0}, headers=h)).json() + assert first["total"] == 5 # the whole visible set, not the page + assert len(first["files"]) == 2 + assert first["limit"] == 2 and first["offset"] == 0 # the window, as applied + second = (await c.get("/uploads", params={"limit": 2, "offset": 2}, headers=h)).json() + third = (await c.get("/uploads", params={"limit": 2, "offset": 4}, headers=h)).json() + assert len(third["files"]) == 1 # the short tail page + ids = [f["file_id"] for page in (first, second, third) for f in page["files"]] + assert len(ids) == 5 and len(set(ids)) == 5 # partitions: no overlap, no gap + + # Stable: the same window twice is the same page. + again = (await c.get("/uploads", params={"limit": 2, "offset": 2}, headers=h)).json() + assert [f["file_id"] for f in again["files"]] == [f["file_id"] for f in second["files"]] + + # Past the end is empty, not an error; the bounds are enforced by the route, not the caller. + assert (await c.get("/uploads", params={"offset": 500}, headers=h)).json()["files"] == [] + assert (await c.get("/uploads", params={"limit": 0}, headers=h)).status_code == 422 + assert (await c.get("/uploads", params={"limit": 501}, headers=h)).status_code == 422 + assert (await c.get("/uploads", params={"offset": -1}, headers=h)).status_code == 422 + + # The audit records the window alongside the unchanged whole-set count -- and no filename. + listings = [a for a in await engine.store.list_audit() if a["action"] == "upload.list"] + details = [json.loads(str(a["detail"])) for a in listings] + # Selected by its window, not by position: list_audit is newest-first, so an index would pin the + # LAST request in this test rather than the first page. + page_one = [d for d in details if d["limit"] == 2 and d["offset"] == 0] + assert page_one, details + assert page_one[0]["count"] == 5 # unchanged meaning: the whole visible set + assert page_one[0]["returned"] == 2 # the window + assert "f0.hl7" not in " ".join(str(a["detail"] or "") for a in listings) + + async def test_upload_denied_for_viewer(engine: Engine, tmp_path: Path) -> None: pytest.importorskip("psutil") from messagefoundry.api import create_app @@ -569,7 +634,9 @@ async def test_uploaded_files_are_owner_only_across_operators( h2 = await _login(c, "op2") lst = await c.get("/uploads", headers=h2) assert lst.status_code == 200 - assert lst.json() == {"total": 0, "files": [], "scope": "own"} + # Whole-response equality on purpose: it says nothing ELSE is in the body either. The window + # fields are the caller's own echo (BACKLOG #1152), so they carry no other operator's data. + assert lst.json() == {"total": 0, "files": [], "scope": "own", "limit": 50, "offset": 0} assert (await c.get(f"/uploads/{fid}/messages", headers=h2)).status_code == 404 rs = await c.post(f"/uploads/{fid}/resend", json={"index": 0, "to": "in1"}, headers=h2) assert rs.status_code == 404, rs.text @@ -675,6 +742,8 @@ async def test_upload_with_no_owner_id_is_reachable_only_with_the_override( "total": 0, "files": [], "scope": "own", + "limit": 50, + "offset": 0, } assert (await c.get(f"/uploads/{fid}/messages", headers=h1)).status_code == 404 assert (await c.delete(f"/uploads/{fid}", headers=h1)).status_code == 404 @@ -729,6 +798,8 @@ async def test_a_recreated_username_cannot_reach_the_departed_operators_upload( "total": 0, "files": [], "scope": "own", + "limit": 50, + "offset": 0, } assert (await c.get(f"/uploads/{fid}/messages", headers=h2)).status_code == 404 assert (await c.delete(f"/uploads/{fid}", headers=h2)).status_code == 404 diff --git a/tests/test_ws_stats_revalidation.py b/tests/test_ws_stats_revalidation.py index 289894764..dbad2a5a2 100644 --- a/tests/test_ws_stats_revalidation.py +++ b/tests/test_ws_stats_revalidation.py @@ -19,6 +19,7 @@ from messagefoundry.api import app as app_module from messagefoundry.api import create_app from messagefoundry.auth import Role +from messagefoundry.auth.identity import ALL_CHANNELS from messagefoundry.auth.service import AuthService from messagefoundry.config.settings import AuthSettings from messagefoundry.pipeline import Engine @@ -48,6 +49,10 @@ async def _add(service: AuthService, username: str, *roles: Role) -> str: roles=[r.value for r in roles], actor="test", ) + # BACKLOG #1152: an unset channel scope now DENIES. Grant the estate explicitly so this + # fixture still stands for an operator who has been provisioned; the channel axis itself + # is exercised in tests/test_channel_rbac.py. + await service.set_channel_scope(user_id, [ALL_CHANNELS], actor="test") user = await service.store.get_user(user_id) assert user is not None and user.password_hash is not None await service.store.set_password(