diff --git a/lib/crates/fabro-server/src/auth/cli_flow.rs b/lib/crates/fabro-server/src/auth/cli_flow.rs index fd8c98368..102aa6175 100644 --- a/lib/crates/fabro-server/src/auth/cli_flow.rs +++ b/lib/crates/fabro-server/src/auth/cli_flow.rs @@ -28,7 +28,7 @@ use url::{Host, Url}; use crate::auth::browser_shell::browser_shell; use crate::auth::{self, AuthCode, ConsumeOutcome, JwtSubject, REFRESH_TOKEN_PREFIX, RefreshToken}; -use crate::jwt_auth::{AuthMode, ConfiguredAuth}; +use crate::jwt_auth::{AuthMode, ConfiguredAuth, bearer_token_from_headers}; use crate::principal_middleware::{AuthContextSlot, AuthStatus, RequestAuth, RequestAuthContext}; use crate::server::AppState; use crate::web_auth::{ @@ -946,18 +946,13 @@ enum RefreshCredential { } fn refresh_credential_from_headers(headers: &HeaderMap) -> RefreshCredential { - let Some(value) = headers.get(header::AUTHORIZATION) else { - return RefreshCredential::Missing; - }; - let Ok(value) = value.to_str() else { - return RefreshCredential::Invalid; - }; - let Some(bearer) = value.strip_prefix("Bearer ") else { - return RefreshCredential::Invalid; - }; - match bearer.strip_prefix(REFRESH_TOKEN_PREFIX) { - Some(secret) => RefreshCredential::Present(secret.to_string()), - None => RefreshCredential::Invalid, + match bearer_token_from_headers(headers) { + None => RefreshCredential::Missing, + Some(Err(_)) => RefreshCredential::Invalid, + Some(Ok(bearer)) => match bearer.strip_prefix(REFRESH_TOKEN_PREFIX) { + Some(secret) => RefreshCredential::Present(secret.to_string()), + None => RefreshCredential::Invalid, + }, } } @@ -1688,11 +1683,14 @@ client_id = "github-client-id" assert_eq!(confirm.status(), StatusCode::SEE_OTHER); let contexts = captured.lock().expect("captured auth contexts").clone(); - assert_eq!(contexts.len(), 3); - for context in contexts { - assert_eq!(context.auth_status, AuthStatus::Authenticated); - assert_eq!(context.principal.display(), "octocat"); - } + let [first, second, third] = <[RequestAuthContext; 3]>::try_from(contexts) + .expect("expected three captured auth contexts"); + assert_eq!(first.auth_status, AuthStatus::Authenticated); + assert_eq!(first.principal.display(), "octocat"); + assert_eq!(second.auth_status, AuthStatus::Authenticated); + assert_eq!(second.principal.display(), "octocat"); + assert_eq!(third.auth_status, AuthStatus::Authenticated); + assert_eq!(third.principal.display(), "octocat"); } #[tokio::test] diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 342bc0c4f..230352f3a 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -122,9 +122,8 @@ use crate::github_webhooks::{ use crate::ip_allowlist::{IpAllowlistConfig, ip_allowlist_middleware}; use crate::jwt_auth::{self, AuthMode}; use crate::principal_middleware::{ - AuthContextSlot, AuthStatus, RequestAuth, RequestAuthContext, RequireCommandLog, - RequireRunBlob, RequireRunScoped, RequireStageArtifact, RequiredUser, principal_middleware, - require_user, + AuthContextSlot, RequestAuth, RequestAuthContext, RequireCommandLog, RequireRunBlob, + RequireRunScoped, RequireStageArtifact, RequiredUser, principal_middleware, require_user, }; use crate::request_id::{self, RequestId}; use crate::run_files::{FilesInFlight, list_run_files, new_files_in_flight}; @@ -975,10 +974,7 @@ pub fn build_router_with_options( .clone() .unwrap_or_else(|| Arc::new(GithubEndpoints::production_defaults())); let webhook_secret = state.server_secret(WEBHOOK_SECRET_ENV); - let demo_principal_layer = - middleware::from_fn_with_state(Arc::clone(&state), principal_middleware); - let real_principal_layer = - middleware::from_fn_with_state(Arc::clone(&state), principal_middleware); + let principal_layer = middleware::from_fn_with_state(Arc::clone(&state), principal_middleware); let api_common = if web_enabled { Router::new() .route("/openapi.json", get(openapi_spec)) @@ -993,7 +989,7 @@ pub fn build_router_with_options( api_common .clone() .merge(demo_routes()) - .layer(demo_principal_layer), + .layer(principal_layer.clone()), ) .layer(axum::Extension(auth_mode.clone())) .layer(axum::Extension(Arc::clone(&github_endpoints))) @@ -1001,7 +997,7 @@ pub fn build_router_with_options( let mut real_router = Router::new().nest( "/api/v1", - api_common.merge(real_routes()).layer(real_principal_layer), + api_common.merge(real_routes()).layer(principal_layer), ); if web_enabled { real_router = real_router.nest("/auth", web_auth::routes().merge(auth::web_routes())); @@ -1402,35 +1398,23 @@ async fn github_webhook( .get("x-hub-signature-256") .and_then(|value| value.to_str().ok()) else { - auth_slot.replace(RequestAuthContext { - principal: Principal::Anonymous, - auth_status: AuthStatus::Invalid, - auth_error_code: Some("unauthorized"), - user_profile: None, - }); + auth_slot.replace(RequestAuthContext::invalid()); warn!(delivery = %delivery_id, "Webhook missing X-Hub-Signature-256 header"); return StatusCode::UNAUTHORIZED; }; if !verify_signature(&secret, &body, signature) { - auth_slot.replace(RequestAuthContext { - principal: Principal::Anonymous, - auth_status: AuthStatus::Invalid, - auth_error_code: Some("unauthorized"), - user_profile: None, - }); + auth_slot.replace(RequestAuthContext::invalid()); warn!(delivery = %delivery_id, "Webhook HMAC signature mismatch"); return StatusCode::UNAUTHORIZED; } - auth_slot.replace(RequestAuthContext { - principal: Principal::Webhook { + auth_slot.replace(RequestAuthContext::authenticated( + Principal::Webhook { delivery_id: delivery_id.to_string(), }, - auth_status: AuthStatus::Authenticated, - auth_error_code: None, - user_profile: None, - }); + None, + )); let event_type = headers .get("x-github-event") diff --git a/lib/crates/fabro-server/src/test_support.rs b/lib/crates/fabro-server/src/test_support.rs index f4c4395fd..a46e6d449 100644 --- a/lib/crates/fabro-server/src/test_support.rs +++ b/lib/crates/fabro-server/src/test_support.rs @@ -1,4 +1,4 @@ -use std::sync::Arc; +use std::sync::{Arc, OnceLock}; use axum::extract::Request; use axum::http::{HeaderValue, header}; @@ -57,12 +57,13 @@ pub fn with_test_user(router: Router) -> Router { async fn inject_test_user_bearer(mut req: Request, next: Next) -> Response { if req.uri().path().starts_with("/api/") && !req.headers().contains_key(header::AUTHORIZATION) { - req.headers_mut().insert( - header::AUTHORIZATION, - HeaderValue::from_static( - "Bearer fabro_dev_abababababababababababababababababababababababababababababababab", - ), - ); + static BEARER: OnceLock = OnceLock::new(); + let bearer = BEARER.get_or_init(|| { + HeaderValue::from_str(&format!("Bearer {TEST_DEV_TOKEN}")) + .expect("dev token bearer header is valid") + }); + req.headers_mut() + .insert(header::AUTHORIZATION, bearer.clone()); } next.run(req).await } diff --git a/lib/crates/fabro-types/src/principal.rs b/lib/crates/fabro-types/src/principal.rs index 618e8d5dc..dc3a84bc6 100644 --- a/lib/crates/fabro-types/src/principal.rs +++ b/lib/crates/fabro-types/src/principal.rs @@ -166,37 +166,64 @@ mod tests { ); } - #[test] - fn round_trips_all_variants() { - let variants = [ - Principal::user(identity(), "octocat".to_string(), AuthMethod::Github), - Principal::Worker { - run_id: fixtures::RUN_1, - }, - Principal::Webhook { - delivery_id: "delivery-1".to_string(), - }, - Principal::Slack { - team_id: "T1".to_string(), - user_id: "U1".to_string(), - user_name: Some("ada".to_string()), - }, - Principal::Agent { - session_id: Some("session".to_string()), - parent_session_id: Some("parent".to_string()), - model: Some("gpt".to_string()), - }, - Principal::System { - system_kind: SystemActorKind::Engine, - }, - Principal::Anonymous, - ]; + #[track_caller] + fn assert_round_trip(principal: &Principal) { + let value = serde_json::to_value(principal).unwrap(); + let parsed: Principal = serde_json::from_value(value).unwrap(); + assert_eq!(&parsed, principal); + } - for principal in variants { - let value = serde_json::to_value(&principal).unwrap(); - let parsed: Principal = serde_json::from_value(value).unwrap(); - assert_eq!(parsed, principal); - } + #[test] + fn round_trips_user_variant() { + assert_round_trip(&Principal::user( + identity(), + "octocat".to_string(), + AuthMethod::Github, + )); + } + + #[test] + fn round_trips_worker_variant() { + assert_round_trip(&Principal::Worker { + run_id: fixtures::RUN_1, + }); + } + + #[test] + fn round_trips_webhook_variant() { + assert_round_trip(&Principal::Webhook { + delivery_id: "delivery-1".to_string(), + }); + } + + #[test] + fn round_trips_slack_variant() { + assert_round_trip(&Principal::Slack { + team_id: "T1".to_string(), + user_id: "U1".to_string(), + user_name: Some("ada".to_string()), + }); + } + + #[test] + fn round_trips_agent_variant() { + assert_round_trip(&Principal::Agent { + session_id: Some("session".to_string()), + parent_session_id: Some("parent".to_string()), + model: Some("gpt".to_string()), + }); + } + + #[test] + fn round_trips_system_variant() { + assert_round_trip(&Principal::System { + system_kind: SystemActorKind::Engine, + }); + } + + #[test] + fn round_trips_anonymous_variant() { + assert_round_trip(&Principal::Anonymous); } #[test] diff --git a/lib/crates/fabro-workflow/src/error.rs b/lib/crates/fabro-workflow/src/error.rs index cdde5086b..f68f216c6 100644 --- a/lib/crates/fabro-workflow/src/error.rs +++ b/lib/crates/fabro-workflow/src/error.rs @@ -276,15 +276,7 @@ impl Error { } pub fn handler_with_anyhow(message: impl Into, source: &anyhow::Error) -> Self { - let message = message.into(); - let causes = source.chain().map(ToString::to_string).collect::>(); - let rendered = render_with_causes(&message, &causes); - let failure_class = classify_failure_reason(&rendered); - Self::Handler { - message, - failure_class, - causes, - } + Self::handler_with_source(message, source.as_ref()) } /// Smart constructor for Engine errors. Classifies the failure reason @@ -315,15 +307,7 @@ impl Error { } pub fn engine_with_anyhow(message: impl Into, source: &anyhow::Error) -> Self { - let message = message.into(); - let causes = source.chain().map(ToString::to_string).collect::>(); - let rendered = render_with_causes(&message, &causes); - let failure_class = classify_failure_reason(&rendered); - Self::Engine { - message, - failure_class, - causes, - } + Self::engine_with_source(message, source.as_ref()) } #[must_use]