From 21caf4e97ef191470953dbea5e9f3b7410a8a3a4 Mon Sep 17 00:00:00 2001 From: fabro Date: Wed, 3 Jun 2026 13:12:17 +0000 Subject: [PATCH] refactor: remove Principal::Anonymous, use Option for absent auth --- .../app/components/run-summary-panel.test.tsx | 2 +- apps/fabro-web/app/lib/principal-display.tsx | 6 --- docs/internal/logging-strategy.md | 2 +- docs/public/api-reference/fabro-api.yaml | 11 ---- .../fabro-api/tests/principal_round_trip.rs | 1 - lib/crates/fabro-server/src/auth/cli_flow.rs | 12 ++--- .../fabro-server/src/principal_middleware.rs | 51 ++++++++++--------- lib/crates/fabro-server/src/server.rs | 17 ++++--- lib/crates/fabro-server/src/web_auth.rs | 2 +- lib/crates/fabro-types/src/principal.rs | 8 --- .../src/.openapi-generator/FILES | 1 - .../fabro-api-client/src/models/index.ts | 1 - .../src/models/principal-anonymous.ts | 25 --------- .../fabro-api-client/src/models/principal.ts | 5 +- .../tests/principal-exhaustive.ts | 2 - 15 files changed, 47 insertions(+), 99 deletions(-) delete mode 100644 lib/packages/fabro-api-client/src/models/principal-anonymous.ts diff --git a/apps/fabro-web/app/components/run-summary-panel.test.tsx b/apps/fabro-web/app/components/run-summary-panel.test.tsx index 52e79cf0f..14fb04510 100644 --- a/apps/fabro-web/app/components/run-summary-panel.test.tsx +++ b/apps/fabro-web/app/components/run-summary-panel.test.tsx @@ -252,7 +252,7 @@ describe("RunSummaryPanelView", () => { }); test("renders non-user actor with kind label", () => { - for (const kind of ["agent", "system", "slack", "webhook", "worker", "anonymous"]) { + for (const kind of ["agent", "system", "slack", "webhook", "worker"]) { const tree = render({ run: makeRun({ created_by: { kind } as any }) }); expect(instanceText(cellAfterLabel(tree, "Created by"))).toContain(kind); } diff --git a/apps/fabro-web/app/lib/principal-display.tsx b/apps/fabro-web/app/lib/principal-display.tsx index fa9d4ec16..666f0f64c 100644 --- a/apps/fabro-web/app/lib/principal-display.tsx +++ b/apps/fabro-web/app/lib/principal-display.tsx @@ -4,7 +4,6 @@ import { ChatBubbleLeftEllipsisIcon, Cog6ToothIcon, CpuChipIcon, - QuestionMarkCircleIcon, ServerIcon, } from "@heroicons/react/20/solid"; import type { Principal } from "@qltysh/fabro-api-client"; @@ -57,10 +56,5 @@ export function principalDisplay(actor: Principal): PrincipalDisplay { return { glyph: principalIconGlyph(), label: "webhook" }; case "worker": return { glyph: principalIconGlyph(), label: "worker" }; - case "anonymous": - return { - glyph: principalIconGlyph(), - label: "anonymous", - }; } } diff --git a/docs/internal/logging-strategy.md b/docs/internal/logging-strategy.md index 63f6f8f54..728a7c031 100644 --- a/docs/internal/logging-strategy.md +++ b/docs/internal/logging-strategy.md @@ -118,7 +118,7 @@ Fields are key-value pairs that make events queryable. Include enough context th | `error` | Error value on failure | | `path` | File system path | | `duration_ms` | Elapsed time in milliseconds | -| `principal_kind` | HTTP caller category (`user`, `worker`, `webhook`, `anonymous`, etc.) | +| `principal_kind` | HTTP caller category (`user`, `worker`, `webhook`, `none`, etc.) | | `auth_status` | HTTP authentication result (`missing`, `invalid`, `expired`, `authenticated`) | | `idp_issuer`, `idp_subject` | Canonical user identity for authenticated user requests | diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 35d7511e5..d075ce98b 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -9014,7 +9014,6 @@ components: - $ref: "#/components/schemas/PrincipalSlack" - $ref: "#/components/schemas/PrincipalAgent" - $ref: "#/components/schemas/PrincipalSystem" - - $ref: "#/components/schemas/PrincipalAnonymous" discriminator: propertyName: kind mapping: @@ -9024,7 +9023,6 @@ components: slack: "#/components/schemas/PrincipalSlack" agent: "#/components/schemas/PrincipalAgent" system: "#/components/schemas/PrincipalSystem" - anonymous: "#/components/schemas/PrincipalAnonymous" PrincipalUser: type: object @@ -9114,15 +9112,6 @@ components: system_kind: $ref: "#/components/schemas/SystemActorKind" - PrincipalAnonymous: - type: object - required: - - kind - properties: - kind: - type: string - enum: [anonymous] - RunEvent: description: > Internal RunEvent-compatible JSON payload. The server validates this diff --git a/lib/crates/fabro-api/tests/principal_round_trip.rs b/lib/crates/fabro-api/tests/principal_round_trip.rs index ca1180e60..601e6372c 100644 --- a/lib/crates/fabro-api/tests/principal_round_trip.rs +++ b/lib/crates/fabro-api/tests/principal_round_trip.rs @@ -118,7 +118,6 @@ fn principal_round_trips_every_variant_through_api_type() { Principal::System { system_kind: SystemActorKind::Watchdog, }, - Principal::Anonymous, ]; for principal in variants { diff --git a/lib/crates/fabro-server/src/auth/cli_flow.rs b/lib/crates/fabro-server/src/auth/cli_flow.rs index 987c2631b..3a9d6e055 100644 --- a/lib/crates/fabro-server/src/auth/cli_flow.rs +++ b/lib/crates/fabro-server/src/auth/cli_flow.rs @@ -1671,11 +1671,11 @@ client_id = "github-client-id" 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!(first.principal.as_ref().unwrap().display(), "octocat"); assert_eq!(second.auth_status, AuthStatus::Authenticated); - assert_eq!(second.principal.display(), "octocat"); + assert_eq!(second.principal.as_ref().unwrap().display(), "octocat"); assert_eq!(third.auth_status, AuthStatus::Authenticated); - assert_eq!(third.principal.display(), "octocat"); + assert_eq!(third.principal.as_ref().unwrap().display(), "octocat"); } #[tokio::test] @@ -2080,7 +2080,7 @@ client_id = "github-client-id" let contexts = captured.lock().expect("captured auth contexts").clone(); assert_eq!(contexts[0].auth_status, AuthStatus::Authenticated); - assert_eq!(contexts[0].principal.display(), "octocat"); + assert_eq!(contexts[0].principal.as_ref().unwrap().display(), "octocat"); assert_eq!(contexts[1].auth_status, AuthStatus::Invalid); assert_eq!( contexts[1].auth_error_code, @@ -2273,8 +2273,8 @@ client_id = "github-client-id" let contexts = captured.lock().expect("captured auth contexts").clone(); assert_eq!(contexts[0].auth_status, AuthStatus::Authenticated); - assert_eq!(contexts[0].principal.display(), "octocat"); - let Principal::User(user) = &contexts[0].principal else { + assert_eq!(contexts[0].principal.as_ref().unwrap().display(), "octocat"); + let Some(Principal::User(user)) = &contexts[0].principal else { panic!("expected user principal"); }; assert_eq!( diff --git a/lib/crates/fabro-server/src/principal_middleware.rs b/lib/crates/fabro-server/src/principal_middleware.rs index f9e8c385c..06a0ba257 100644 --- a/lib/crates/fabro-server/src/principal_middleware.rs +++ b/lib/crates/fabro-server/src/principal_middleware.rs @@ -19,7 +19,7 @@ use crate::worker_token::{self, WORKER_TOKEN_KID, WorkerScopeSet}; #[derive(Clone, Debug)] pub(crate) struct RequestAuthContext { - pub principal: Principal, + pub principal: Option, pub auth_status: AuthStatus, pub auth_error_code: Option, pub user_profile: Option, @@ -76,7 +76,7 @@ impl RequestAuthContext { #[must_use] pub(crate) fn initial() -> Self { Self { - principal: Principal::Anonymous, + principal: None, auth_status: AuthStatus::Missing, auth_error_code: None, user_profile: None, @@ -87,7 +87,7 @@ impl RequestAuthContext { #[must_use] pub(crate) fn authenticated(principal: Principal, user_profile: Option) -> Self { Self { - principal, + principal: Some(principal), auth_status: AuthStatus::Authenticated, auth_error_code: None, user_profile, @@ -98,7 +98,7 @@ impl RequestAuthContext { #[must_use] pub(crate) fn authenticated_worker(run_id: RunId, scopes: WorkerScopeSet) -> Self { Self { - principal: Principal::Worker { run_id }, + principal: Some(Principal::Worker { run_id }), auth_status: AuthStatus::Authenticated, auth_error_code: None, user_profile: None, @@ -125,7 +125,7 @@ impl RequestAuthContext { #[must_use] pub(crate) fn rejected(status: AuthStatus, code: Option) -> Self { Self { - principal: Principal::Anonymous, + principal: None, auth_status: status, auth_error_code: code, user_profile: None, @@ -148,7 +148,7 @@ impl AuthStatus { #[derive(Clone, Debug)] pub(crate) struct RequestAuthLogContext { - pub principal: Principal, + pub principal: Option, pub auth_status: AuthStatus, pub auth_error_code: Option, } @@ -172,7 +172,10 @@ impl AuthContextSlot { pub(crate) fn log_snapshot(&self) -> RequestAuthLogContext { let context = self.0.lock().expect("auth context lock poisoned"); RequestAuthLogContext { - principal: principal_without_log_unused_fields(&context.principal), + principal: context + .principal + .as_ref() + .map(principal_without_log_unused_fields), auth_status: context.auth_status, auth_error_code: context.auth_error_code, } @@ -402,7 +405,7 @@ fn auth_slot_from_parts(parts: &Parts) -> AuthContextSlot { pub(crate) fn require_user(slot: &AuthContextSlot) -> Result { let context = slot.0.lock().expect("auth context lock poisoned"); match &context.principal { - Principal::User(user) => Ok(user.clone()), + Some(Principal::User(user)) => Ok(user.clone()), _ => Err(auth_rejection(context.auth_status, context.auth_error_code)), } } @@ -412,7 +415,7 @@ pub(crate) fn require_authenticated_user( ) -> Result { let context = slot.snapshot(); match context.principal { - Principal::User(principal) => { + Some(Principal::User(principal)) => { let Some(profile) = context.user_profile else { return Err(ApiError::new( StatusCode::INTERNAL_SERVER_ERROR, @@ -428,11 +431,11 @@ pub(crate) fn require_authenticated_user( pub(crate) fn require_run_management_actor(slot: &AuthContextSlot) -> Result { let context = slot.0.lock().expect("auth context lock poisoned"); match &context.principal { - Principal::User(user) => Ok(Principal::User(user.clone())), - Principal::Worker { run_id } if context.worker_scopes.has_agent_run_tools() => { + Some(Principal::User(user)) => Ok(Principal::User(user.clone())), + Some(Principal::Worker { run_id }) if context.worker_scopes.has_agent_run_tools() => { Ok(Principal::Worker { run_id: *run_id }) } - Principal::Worker { .. } => Err(ApiError::forbidden()), + Some(Principal::Worker { .. }) => Err(ApiError::forbidden()), _ => Err(auth_rejection(context.auth_status, context.auth_error_code)), } } @@ -443,9 +446,9 @@ fn require_worker_or_user_for_run( ) -> Result<(), ApiError> { let context = slot.0.lock().expect("auth context lock poisoned"); match &context.principal { - Principal::User(_) => Ok(()), - Principal::Worker { run_id } if run_id == route_run_id => Ok(()), - Principal::Worker { .. } => Err(ApiError::forbidden()), + Some(Principal::User(_)) => Ok(()), + Some(Principal::Worker { run_id }) if run_id == route_run_id => Ok(()), + Some(Principal::Worker { .. }) => Err(ApiError::forbidden()), _ => Err(auth_rejection(context.auth_status, context.auth_error_code)), } } @@ -453,8 +456,8 @@ fn require_worker_or_user_for_run( fn require_worker_for_run(slot: &AuthContextSlot, route_run_id: &RunId) -> Result<(), ApiError> { let context = slot.0.lock().expect("auth context lock poisoned"); match &context.principal { - Principal::Worker { run_id } if run_id == route_run_id => Ok(()), - Principal::Worker { .. } | Principal::User(_) => Err(ApiError::forbidden()), + Some(Principal::Worker { run_id }) if run_id == route_run_id => Ok(()), + Some(Principal::Worker { .. } | Principal::User(_)) => Err(ApiError::forbidden()), _ => Err(auth_rejection(context.auth_status, context.auth_error_code)), } } @@ -465,13 +468,13 @@ fn require_run_management_target( ) -> Result { let context = slot.0.lock().expect("auth context lock poisoned"); match &context.principal { - Principal::User(user) => Ok(Principal::User(user.clone())), - Principal::Worker { run_id } + Some(Principal::User(user)) => Ok(Principal::User(user.clone())), + Some(Principal::Worker { run_id }) if run_id == route_run_id || context.worker_scopes.has_agent_run_tools() => { Ok(Principal::Worker { run_id: *run_id }) } - Principal::Worker { .. } => Err(ApiError::forbidden()), + Some(Principal::Worker { .. }) => Err(ApiError::forbidden()), _ => Err(auth_rejection(context.auth_status, context.auth_error_code)), } } @@ -687,7 +690,7 @@ mod tests { let context = classify_request(&request, state.as_ref()); assert_eq!(context.auth_status, AuthStatus::Authenticated); - assert!(matches!(context.principal, Principal::User(_))); + assert!(matches!(context.principal, Some(Principal::User(_)))); assert!(context.user_profile.is_some()); } @@ -727,7 +730,7 @@ mod tests { let context = classify_request(&request, state.as_ref()); assert_eq!(context.auth_status, AuthStatus::Authenticated); - assert_eq!(context.principal, Principal::Worker { run_id }); + assert_eq!(context.principal, Some(Principal::Worker { run_id })); assert!(!context.worker_scopes.has_agent_run_tools()); } @@ -746,7 +749,7 @@ mod tests { let context = classify_request(&request, state.as_ref()); assert_eq!(context.auth_status, AuthStatus::Authenticated); - assert_eq!(context.principal, Principal::Worker { run_id }); + assert_eq!(context.principal, Some(Principal::Worker { run_id })); assert!(context.worker_scopes.has_agent_run_tools()); } @@ -825,7 +828,7 @@ mod tests { assert_eq!(context.auth_status, AuthStatus::Missing); assert_eq!(context.auth_error_code, None); - assert_eq!(context.principal, Principal::Anonymous); + assert_eq!(context.principal, None); } #[test] diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 7bff4968f..4710c40ca 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -1876,7 +1876,10 @@ async fn http_log_middleware(mut req: axum_extract::Request, next: Next) -> Resp let status = response.status().as_u16(); let latency_ms = start.elapsed().as_millis(); let auth_context = auth_slot.log_snapshot(); - let principal_kind = auth_context.principal.kind(); + let principal_kind = auth_context + .principal + .as_ref() + .map_or("none", Principal::kind); let auth_status = auth_context.auth_status.as_str(); macro_rules! emit_http_log { @@ -1914,27 +1917,27 @@ async fn http_log_middleware(mut req: axum_extract::Request, next: Next) -> Resp macro_rules! emit_principal_http_log { ($level:ident) => {{ match &auth_context.principal { - Principal::User(user) => emit_http_log!( + Some(Principal::User(user)) => emit_http_log!( $level, user_auth_method = user.auth_method.as_str(), idp_issuer = user.identity.issuer(), idp_subject = user.identity.subject(), login = user.login.as_str(), ), - Principal::Worker { run_id } => { + Some(Principal::Worker { run_id }) => { emit_http_log!($level, run_id = run_id.to_string().as_str(),) } - Principal::Webhook { delivery_id } => { + Some(Principal::Webhook { delivery_id }) => { emit_http_log!($level, delivery_id = delivery_id.as_str(),) } - Principal::Slack { + Some(Principal::Slack { team_id, user_id, .. - } => emit_http_log!( + }) => emit_http_log!( $level, team_id = team_id.as_str(), user_id = user_id.as_str(), ), - Principal::Agent { .. } | Principal::System { .. } | Principal::Anonymous => { + None | Some(Principal::Agent { .. } | Principal::System { .. }) => { emit_http_log!($level) } } diff --git a/lib/crates/fabro-server/src/web_auth.rs b/lib/crates/fabro-server/src/web_auth.rs index 033e46db6..e231599b2 100644 --- a/lib/crates/fabro-server/src/web_auth.rs +++ b/lib/crates/fabro-server/src/web_auth.rs @@ -1365,7 +1365,7 @@ client_id = "github-client-id" let contexts = captured.lock().expect("captured auth contexts").clone(); assert_eq!(contexts[0].auth_status, AuthStatus::Authenticated); - assert!(matches!(contexts[0].principal, Principal::User(_))); + assert!(matches!(contexts[0].principal, Some(Principal::User(_)))); assert_eq!(contexts[1].auth_status, AuthStatus::Invalid); assert_eq!( contexts[1].auth_error_code, diff --git a/lib/crates/fabro-types/src/principal.rs b/lib/crates/fabro-types/src/principal.rs index 2c0ff3807..4f1962ea2 100644 --- a/lib/crates/fabro-types/src/principal.rs +++ b/lib/crates/fabro-types/src/principal.rs @@ -39,7 +39,6 @@ pub enum Principal { System { system_kind: SystemActorKind, }, - Anonymous, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, IntoStaticStr)] @@ -97,7 +96,6 @@ impl Principal { Self::Slack { .. } => "slack", Self::Agent { .. } => "agent", Self::System { .. } => "system", - Self::Anonymous => "anonymous", } } @@ -123,7 +121,6 @@ impl Principal { } => session_id.clone(), Self::Agent { .. } => "agent".to_string(), Self::System { system_kind } => format!("system:{system_kind}"), - Self::Anonymous => "anonymous".to_string(), } } } @@ -291,11 +288,6 @@ mod tests { }); } - #[test] - fn round_trips_anonymous_variant() { - assert_round_trip(&Principal::Anonymous); - } - #[test] fn auth_method_as_str_matches_serde() { assert_eq!(AuthMethod::Github.as_str(), "github"); diff --git a/lib/packages/fabro-api-client/src/.openapi-generator/FILES b/lib/packages/fabro-api-client/src/.openapi-generator/FILES index 5e97116d9..7839b95b6 100644 --- a/lib/packages/fabro-api-client/src/.openapi-generator/FILES +++ b/lib/packages/fabro-api-client/src/.openapi-generator/FILES @@ -271,7 +271,6 @@ models/preflight-workflow-summary.ts models/preview-url-request.ts models/preview-url-response.ts models/principal-agent.ts -models/principal-anonymous.ts models/principal-slack.ts models/principal-system.ts models/principal-user.ts diff --git a/lib/packages/fabro-api-client/src/models/index.ts b/lib/packages/fabro-api-client/src/models/index.ts index 87b9c189c..af8ae77e0 100644 --- a/lib/packages/fabro-api-client/src/models/index.ts +++ b/lib/packages/fabro-api-client/src/models/index.ts @@ -244,7 +244,6 @@ export * from './preview-url-request'; export * from './preview-url-response'; export * from './principal'; export * from './principal-agent'; -export * from './principal-anonymous'; export * from './principal-slack'; export * from './principal-system'; export * from './principal-user'; diff --git a/lib/packages/fabro-api-client/src/models/principal-anonymous.ts b/lib/packages/fabro-api-client/src/models/principal-anonymous.ts deleted file mode 100644 index daac61df0..000000000 --- a/lib/packages/fabro-api-client/src/models/principal-anonymous.ts +++ /dev/null @@ -1,25 +0,0 @@ -/* tslint:disable */ -/* eslint-disable */ -/** - * Fabro Run API - * HTTP API for managing Fabro workflow run executions. - * - * The version of the OpenAPI document: 0.1.0 - * - * - * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). - * https://openapi-generator.tech - * Do not edit the class manually. - */ - - - -export interface PrincipalAnonymous { - 'kind': PrincipalAnonymousKindEnum; -} - -export const PrincipalAnonymousKindEnum = { - ANONYMOUS: 'anonymous' -} as const; - -export type PrincipalAnonymousKindEnum = typeof PrincipalAnonymousKindEnum[keyof typeof PrincipalAnonymousKindEnum]; diff --git a/lib/packages/fabro-api-client/src/models/principal.ts b/lib/packages/fabro-api-client/src/models/principal.ts index e3597295d..08b5422df 100644 --- a/lib/packages/fabro-api-client/src/models/principal.ts +++ b/lib/packages/fabro-api-client/src/models/principal.ts @@ -24,9 +24,6 @@ import type { IdpIdentity } from './idp-identity'; import type { PrincipalAgent } from './principal-agent'; // May contain unused imports in some cases // @ts-ignore -import type { PrincipalAnonymous } from './principal-anonymous'; -// May contain unused imports in some cases -// @ts-ignore import type { PrincipalSlack } from './principal-slack'; // May contain unused imports in some cases // @ts-ignore @@ -47,4 +44,4 @@ import type { SystemActorKind } from './system-actor-kind'; /** * @type Principal */ -export type Principal = { kind: 'agent' } & PrincipalAgent | { kind: 'anonymous' } & PrincipalAnonymous | { kind: 'slack' } & PrincipalSlack | { kind: 'system' } & PrincipalSystem | { kind: 'user' } & PrincipalUser | { kind: 'webhook' } & PrincipalWebhook | { kind: 'worker' } & PrincipalWorker; +export type Principal = { kind: 'agent' } & PrincipalAgent | { kind: 'slack' } & PrincipalSlack | { kind: 'system' } & PrincipalSystem | { kind: 'user' } & PrincipalUser | { kind: 'webhook' } & PrincipalWebhook | { kind: 'worker' } & PrincipalWorker; diff --git a/lib/packages/fabro-api-client/tests/principal-exhaustive.ts b/lib/packages/fabro-api-client/tests/principal-exhaustive.ts index 114b1e640..5fff8b8f4 100644 --- a/lib/packages/fabro-api-client/tests/principal-exhaustive.ts +++ b/lib/packages/fabro-api-client/tests/principal-exhaustive.ts @@ -12,8 +12,6 @@ export function principalKind(principal: Principal): string { switch (principal.kind) { case "agent": return "agent"; - case "anonymous": - return "anonymous"; case "slack": return "slack"; case "system":