scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-10-07 03:00:29 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

refactor: remove Principal::Anonymous, use Option<Principal> for absent auth

Browse source
This commit is contained in:
fabro 2026-06-03 13:12:17 +00:00
parent f26ad95088
commit 21caf4e97e
15 changed files with 47 additions and 99 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
apps/fabro-web/app/components/run-summary-panel.test.tsx
View file

@ -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);
}

6
apps/fabro-web/app/lib/principal-display.tsx
View file

@ -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(<BoltIcon className="size-3" />), label: "webhook" };
case "worker":
return { glyph: principalIconGlyph(<ServerIcon className="size-3" />), label: "worker" };
case "anonymous":
return {
glyph: principalIconGlyph(<QuestionMarkCircleIcon className="size-3" />),
label: "anonymous",
};
}
}

2
docs/internal/logging-strategy.md
View file

@ -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 |

11
docs/public/api-reference/fabro-api.yaml
View file

@ -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

1
lib/crates/fabro-api/tests/principal_round_trip.rs
View file

@ -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 {

12
lib/crates/fabro-server/src/auth/cli_flow.rs
View file

@ -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!(

51
lib/crates/fabro-server/src/principal_middleware.rs
View file

@ -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<Principal>,
pub auth_status: AuthStatus,
pub auth_error_code: Option<AuthErrorCode>,
pub user_profile: Option<UserProfile>,
@ -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<UserProfile>) -> 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<AuthErrorCode>) -> 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<Principal>,
pub auth_status: AuthStatus,
pub auth_error_code: Option<AuthErrorCode>,
}
@ -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<UserPrincipal, ApiError> {
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<AuthenticatedUser, ApiError> {
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<Principal, ApiError> {
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<Principal, ApiError> {
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]

17
lib/crates/fabro-server/src/server.rs
View file

@ -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)
}
}

2
lib/crates/fabro-server/src/web_auth.rs
View file

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

8
lib/crates/fabro-types/src/principal.rs
View file

@ -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");

1
lib/packages/fabro-api-client/src/.openapi-generator/FILES generated
View file

@ -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

1
lib/packages/fabro-api-client/src/models/index.ts generated
View file

@ -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';

25
lib/packages/fabro-api-client/src/models/principal-anonymous.ts generated
View file

@ -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];

5
lib/packages/fabro-api-client/src/models/principal.ts generated
View file

@ -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;

2
lib/packages/fabro-api-client/tests/principal-exhaustive.ts
View file

@ -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":