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, [