From 1ed0bc57ef8338294ca274dc20b7270f00b22c58 Mon Sep 17 00:00:00 2001 From: Dragan Spiridonov Date: Thu, 23 Jul 2026 14:31:01 +0200 Subject: [PATCH] docs(adr): correct a false claim about browser scope; pin the decision in code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cross-vendor pre-merge sweep found no merge blockers, but did surface an error in ADR-271 that I wrote — and a coherence gap behind it. ADR-271 P2 stated that capping the browser session at `sensing:read` was "considered and rejected, because the dashboard genuinely performs admin operations". That is wrong. `/oauth/start` (main.rs:9206) already requests `SENSING_READ` and nothing else, deliberately, with a comment saying so. The browser session is ALREADY read-only, so the breakage I claimed capping would cause is simply the current behaviour. Two consequences, now stated in the ADR instead of left to be discovered: 1. The UI's admin controls do not work from a browser OAuth session. `model.service.js:136` issues DELETE /api/v1/models/{id}, which 401s. Admin work needs the CLI (`login --admin`) or a pasted admin bearer. A gap in a new feature, not a regression — the token-paste path is unchanged. 2. The ADMIN_REVERIFY_SECS step-up control added in the previous commit guards a case that cannot currently arise. No browser session holds `sensing:admin`, so the freshness branch never fires in production. Its tests pass because the crate-internal seam mints an admin cookie the real flow never produces. That second point is worth being blunt about: it is the same shape as several defects this branch already fixed — correct code, green tests, unreachable call site. The difference is that here the guard is deliberately ahead of the need rather than mistakenly behind it, and saying which one it is matters. So the constant is now named `BROWSER_SIGNIN_SCOPE` rather than inlined, with the cost of widening it documented at the definition, and two tests: `browser_sign_in_stays_read_only_until_someone_decides_otherwise` pins the value, and `the_authorize_url_actually_carries_that_scope` proves it reaches the wire — asserting on the constant alone would pass even if `begin` were called with something else, which is exactly the isolation failure being guarded against. The ADR also records the coherent way to add browser-side admin if wanted: escalate-on-demand via the RFC 6750 challenge, keeping least privilege by default rather than asking every user to consent to delete capability to watch a stream. Not bundled here — it needs a scope parameter and a UI affordance. Sweep verdict: no merge blockers. Two other non-blocking risks it raised are accurate and unchanged: concurrent JWKS refresh at the stale boundary is not atomic (a duplicated idempotent GET, already documented as an accepted cost), and the service worker's SHELL_ASSETS use root-relative paths while the UI mounts under /ui, so offline shell precaching is incomplete — pre-existing, unrelated to this branch. Verified: workspace 176 suites clean. Co-Authored-By: Ruflo & AQE --- .../ADR-271-cognitum-oauth-resource-server.md | 40 ++++++++++++++-- .../src/browser_session.rs | 46 +++++++++++++++++++ .../wifi-densepose-sensing-server/src/main.rs | 5 +- 3 files changed, 86 insertions(+), 5 deletions(-) diff --git a/docs/adr/ADR-271-cognitum-oauth-resource-server.md b/docs/adr/ADR-271-cognitum-oauth-resource-server.md index 60e16f50..7c50da64 100644 --- a/docs/adr/ADR-271-cognitum-oauth-resource-server.md +++ b/docs/adr/ADR-271-cognitum-oauth-resource-server.md @@ -295,9 +295,43 @@ authority that cannot be revoked. Cognitum publishes no introspection endpoint behind a session still stands. A disabled account keeps sensing access, and `sensing:admin` if it had it, until the cookie expires on its own. -Capping the session at `sensing:read` was considered and **rejected**: the -dashboard genuinely performs admin operations (`model.service.js:136` issues -`DELETE /api/v1/models/{id}`), so that would break shipped functionality. +**Correction.** An earlier revision of this section said capping the session at +`sensing:read` was "considered and rejected, because the dashboard genuinely +performs admin operations". That was wrong, and a cross-vendor pre-merge sweep +caught it: `/oauth/start` (`main.rs:9206`) already requests `SENSING_READ` and +nothing else, deliberately — "admin work goes through the CLI, which requires an +explicit `--admin`". So a browser session is **already** read-only, and the +consequence I claimed capping would cause is simply the current behaviour. + +Two things follow, and both are stated here rather than left for the next reader +to trip over: + +1. **The UI's admin controls do not work from a browser OAuth session.** + `model.service.js:136` issues `DELETE /api/v1/models/{id}`; from a + Cognitum-signed-in browser that returns 401. Admin work requires either the + CLI (`wifi-densepose login --admin`) or a manually pasted admin bearer in the + QuickSettings token field. This is a gap in the browser feature, not a + regression — browser sign-in is new here, and the token-paste path still + carries whatever authority the pasted token has. + +2. **The step-up control below is therefore a guard ahead of need, not an active + one.** No browser session currently holds `sensing:admin`, so + `session.has_scope(SENSING_ADMIN)` is false and the freshness branch never + fires in production. Its tests pass because the crate-internal test seam + mints an admin cookie the real flow does not produce. That is worth naming + plainly: it is correct code guarding a case that cannot yet arise, and it + becomes load-bearing the moment anyone widens the requested scope — which is + the right time for the guard to already exist, but it is not evidence that + the control is exercised today. + +If browser-side admin is wanted, the coherent design is **escalate on demand**: +keep `sensing:read` as the default, and have the RFC 6750 challenge send the +user back through `/oauth/start` with `sensing:admin` requested, returning a +session that holds admin AND a fresh `auth_time`. That preserves least privilege +by default, makes the challenge meaningful, and is the only option that does not +ask every user to consent to delete capability just to watch a stream. It needs +a scope parameter on `/oauth/start` and a UI affordance, so it is deliberately +not bundled into this change. **Three options, with the tradeoff each carries:** diff --git a/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs b/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs index b863bf52..917fc8f1 100644 --- a/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs +++ b/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs @@ -81,6 +81,27 @@ pub const SESSION_TTL_SECS: i64 = 3600; /// fresh `auth_time` and, if their Cognitum session is live, no prompt. pub const ADMIN_REVERIFY_SECS: i64 = 300; +/// The scope `/oauth/start` requests. Read-only, deliberately. +/// +/// Named rather than inlined because it is a decision, not a detail, and it has +/// two consequences that are easy to widen by accident: +/// +/// 1. **The UI's admin controls do not work from a browser session.** +/// `model.service.js` issues `DELETE /api/v1/models/{id}`; from a +/// Cognitum-signed-in browser that is a 401. Admin work goes through the CLI +/// (`wifi-densepose login --admin`) or a pasted admin bearer. +/// 2. **[`ADMIN_REVERIFY_SECS`] therefore guards a case that cannot yet arise.** +/// No browser session holds `sensing:admin`, so the freshness branch never +/// fires in production today. It becomes load-bearing the instant this +/// constant grows, which is the right ordering — but do not mistake its +/// passing tests for evidence that the control is exercised. +/// +/// Widening this to include `sensing:admin` would make every browser sign-in +/// consent to delete capability just to watch a stream. The coherent way to add +/// browser-side admin is escalate-on-demand: keep this read-only and let the +/// RFC 6750 challenge send the user back through `/oauth/start` asking for more. +pub const BROWSER_SIGNIN_SCOPE: &str = ruview_auth::scope::SENSING_READ; + fn now() -> i64 { SystemTime::now() .duration_since(UNIX_EPOCH) @@ -785,6 +806,31 @@ mod tests { )); } + #[test] + fn browser_sign_in_stays_read_only_until_someone_decides_otherwise() { + // Pins the decision documented on BROWSER_SIGNIN_SCOPE. Widening it is + // legitimate, but it must be a choice: it makes every browser sign-in + // consent to delete capability, and it activates the ADMIN_REVERIFY_SECS + // branch that is currently unreachable in production. + assert_eq!(BROWSER_SIGNIN_SCOPE, ruview_auth::scope::SENSING_READ); + assert!( + !BROWSER_SIGNIN_SCOPE.split_whitespace().any(|s| s == ruview_auth::scope::SENSING_ADMIN), + "browser sign-in must not silently request admin: {BROWSER_SIGNIN_SCOPE}" + ); + } + + #[test] + fn the_authorize_url_actually_carries_that_scope() { + // The constant is only worth pinning if it reaches the wire. Asserting + // on the constant alone would pass even if `begin` were called with + // something else — the same "tested in isolation, call site untested" + // shape that produced several defects in this branch. + init_secret_for_tests(); + let (url, _) = begin("https://a.example", "ruview", BROWSER_SIGNIN_SCOPE, false).unwrap(); + assert!(url.contains("scope=sensing%3Aread"), "{url}"); + assert!(!url.contains("sensing%3Aadmin"), "{url}"); + } + #[test] fn clearing_cookies_expires_them_immediately() { for c in [clear_session(false), clear_transaction(false)] { diff --git a/v2/crates/wifi-densepose-sensing-server/src/main.rs b/v2/crates/wifi-densepose-sensing-server/src/main.rs index fcde93ea..0e6be4a2 100644 --- a/v2/crates/wifi-densepose-sensing-server/src/main.rs +++ b/v2/crates/wifi-densepose-sensing-server/src/main.rs @@ -9202,8 +9202,9 @@ async fn oauth_start( }; let secure = request_is_tls(&headers); // Least privilege: a browser session asks for read. Admin work goes through - // the CLI, which requires an explicit --admin. - match bs::begin(&issuer, &auth.primary_client_id(), ruview_auth::scope::SENSING_READ, secure) { + // the CLI, which requires an explicit --admin. See BROWSER_SIGNIN_SCOPE for + // what widening this would cost. + match bs::begin(&issuer, &auth.primary_client_id(), bs::BROWSER_SIGNIN_SCOPE, secure) { Ok((location, cookie)) => ( axum::http::StatusCode::FOUND, [