mirror of
https://github.com/ruvnet/RuView
synced 2026-07-23 17:33:20 +00:00
docs(adr): correct a false claim about browser scope; pin the decision in code
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
This commit is contained in:
@@ -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:**
|
||||
|
||||
|
||||
@@ -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)] {
|
||||
|
||||
@@ -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,
|
||||
[
|
||||
|
||||
Reference in New Issue
Block a user