berkus
/
wifi-densepose
mirror of https://github.com/ruvnet/wifi-densepose.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(auth): check scope before step-up, so read-only callers are not sent in a circle 

Found by an empirical route sweep during the pre-merge pass, not by a test.

The step-up gate fired BEFORE the scope check, so a caller holding only
`sensing:read` who attempted a privileged action with a session older than
ADMIN_REVERIFY_SECS received the RFC 6750 "reauthentication required"
challenge. `api.service.js` acts on that by redirecting through /oauth/start —
and the user returns with exactly the same scopes and is refused again. One
wasted round trip, and a reason for the refusal that was simply untrue: they
were not refused for staleness, they were refused for capability.

Now the scope check comes first, so only a caller who actually HOLDS
`sensing:admin` is ever asked to prove the session is fresh. Not a loop before
(the second refusal is a plain 401 with no challenge) and not a security issue
either way — both paths refuse. It was misleading, and it cost a redirect.

Also confirms the deny-by-default gate does not over-reach. Probed the real
binary with auth off vs on:

  /, /ui/*, /health, /health/{live,ready,metrics,version},
  /oauth/{status,start}    unchanged
  /api/field, /api/v1/*    200 -> 401   (intended)
  /metrics, /favicon.ico   404 -> 401   (never existed; now uniform)

Nothing that returned 2xx before returns 401 now, which is the property that
matters for existing deployments.

Tests: +2. `a_read_only_user_is_not_sent_to_reauthenticate_pointlessly` fails
against the old ordering with the challenge header present; its counterpart
`an_admin_holder_with_a_stale_session_does_get_the_challenge` ensures the fix
does not simply disable the signal.

Verified: workspace 176 suites clean, ruview-auth 62+25+2 --all-features.

Co-Authored-By: Ruflo & AQE
This commit is contained in:
Dragan Spiridonov 2026-07-23 14:03:54 +02:00
parent 89cceaf835
commit f7cc68bd5c
1 changed files with 85 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

87
v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs
View File

@ -667,7 +667,16 @@ pub async fn require_bearer(
// token that created it and Cognitum offers no introspection,
// so a stale session is authority we cannot revoke — bounded
// here to the routes where that authority actually does damage.
if required == scope::SENSING_ADMIN && !session.recently_authenticated() {
//
// Ordered AFTER the scope check on purpose. Asking someone who
// does not hold `sensing:admin` to re-authenticate sends them
// through a redirect that cannot possibly help: they come back
// with the same scopes and are refused again. Only a caller who
// actually holds the capability is asked to prove it is fresh.
if session.has_scope(required)
&& required == scope::SENSING_ADMIN
&& !session.recently_authenticated()
{
tracing::debug!(
sub = %session.subject,
path = %path.as_str(),
@ -716,7 +725,16 @@ async fn session_or_unauthorized(auth: &AuthState, request: Request, next: Next)
// token that created it and Cognitum offers no introspection,
// so a stale session is authority we cannot revoke — bounded
// here to the routes where that authority actually does damage.
if required == scope::SENSING_ADMIN && !session.recently_authenticated() {
//
// Ordered AFTER the scope check on purpose. Asking someone who
// does not hold `sensing:admin` to re-authenticate sends them
// through a redirect that cannot possibly help: they come back
// with the same scopes and are refused again. Only a caller who
// actually holds the capability is asked to prove it is fresh.
if session.has_scope(required)
&& required == scope::SENSING_ADMIN
&& !session.recently_authenticated()
{
tracing::debug!(
sub = %session.subject,
path = %path.as_str(),
@ -1347,6 +1365,71 @@ mod oauth_tests {
}
}
#[tokio::test]
async fn a_read_only_user_is_not_sent_to_reauthenticate_pointlessly() {
// The scope check must come FIRST. A caller without `sensing:admin`
// cannot be helped by re-authenticating — they return with the same
// scopes and are refused again — so sending the step-up challenge would
// cost them a redirect and tell them something untrue about why they
// were refused. Only a caller who actually HOLDS the capability is
// asked to prove it is fresh.
let stale_read_only = format!(
"ruview_session={}",
crate::browser_session::test_cookie_value_aged(
"sub-b",
"acct-b",
scope::SENSING_READ,
3600,
crate::browser_session::ADMIN_REVERIFY_SECS + 60,
)
);
let req = Request::builder()
.method("DELETE")
.uri("/api/v1/models/m1")
.header(axum::http::header::COOKIE, &stale_read_only);
let resp = app(oauth_only())
.oneshot(req.body(Body::empty()).unwrap())
.await
.unwrap();
assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
let challenge = resp
.headers()
.get(axum::http::header::WWW_AUTHENTICATE)
.and_then(|v| v.to_str().ok())
.unwrap_or_default();
assert!(
!challenge.contains("reauthentication required"),
"a read-only caller must not be told to re-authenticate: {challenge:?}"
);
}
#[tokio::test]
async fn an_admin_holder_with_a_stale_session_does_get_the_challenge() {
// The counterpart: the signal must actually fire for the caller it can
// help, or the UI never learns to send them back through /oauth/start.
let c = aged_admin_cookie(crate::browser_session::ADMIN_REVERIFY_SECS + 60);
let req = Request::builder()
.method("DELETE")
.uri("/api/v1/models/m1")
.header(axum::http::header::COOKIE, &c);
let resp = app(oauth_only())
.oneshot(req.body(Body::empty()).unwrap())
.await
.unwrap();
assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
let challenge = resp
.headers()
.get(axum::http::header::WWW_AUTHENTICATE)
.and_then(|v| v.to_str().ok())
.unwrap_or_default();
assert!(
challenge.contains("reauthentication required"),
"an admin holder with a stale session must be told to re-authenticate: {challenge:?}"
);
}
#[tokio::test]
async fn a_freshly_authenticated_session_can_delete() {
// The negative above must not pass merely because admin never works.