From e03bee2c8d43ea65ac2748fd5d579b056b31aff9 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Wed, 9 Sep 2026 14:46:40 -0400 Subject: [PATCH] fix(oauth)!: keep a refusal's rule and its reason apart They were joined into one "rule: reason" string, which left an adapter to split on a colon to render either. `reason` is its own optional field now, present only on a deny, and `rule` is the identifier alone. Co-Authored-By: Claude Fable 5.1 Change-Id: Ia463f67c332bdb10c536bc1461535eb037ccd02e --- crates/didbot-agentd/src/bin/didbot.rs | 20 ++++++++++ crates/didbot-agentd/src/decisions.rs | 23 ++++++++---- crates/didbot-agentd/src/protocol.rs | 51 +++++++++++++++++++++++++- 3 files changed, 85 insertions(+), 9 deletions(-) diff --git a/crates/didbot-agentd/src/bin/didbot.rs b/crates/didbot-agentd/src/bin/didbot.rs index 70e5606c..4594d126 100644 --- a/crates/didbot-agentd/src/bin/didbot.rs +++ b/crates/didbot-agentd/src/bin/didbot.rs @@ -126,6 +126,11 @@ fn one_line(decision: &DecisionForAgent) -> String { if let Some(rule) = &decision.rule { line.push_str(&format!(" rule={rule:?}")); } + // After the rule, because it is the rule's own sentence: an owner reads + // the identifier, an agent reads the prose. + if let Some(reason) = &decision.reason { + line.push_str(&format!(" reason={reason:?}")); + } line.push_str(&format!(" expires={}", decision.expires_at)); match &decision.token { Some(token) => line.push_str(&format!(" token={token}")), @@ -300,6 +305,7 @@ mod tests { granted: vec!["atproto".into()], cut: vec!["repo:com.example.thing".into()], rule: Some("ceiling".into()), + reason: None, verdict: "narrow".into(), expires_at: "2026-09-09T12:04:00Z".into(), } @@ -325,9 +331,21 @@ mod tests { refused.token = None; refused.verdict = "deny".into(); refused.granted = Vec::new(); + refused.cut = Vec::new(); + refused.rule = Some("app-allowlist".into()); + refused.reason = Some("that client is not admitted".into()); let line = one_line(&refused); assert!(line.contains("verdict=deny"), "{line}"); assert!(line.contains("granted=none"), "{line}"); + // The rule and the reason are two fields, and the reason follows the + // rule it belongs to. + let rule_at = line.find("rule=").expect("the rule"); + let reason_at = line.find("reason=").expect("the reason"); + assert!(rule_at < reason_at, "{line}"); + assert!( + line.contains(r#"reason="that client is not admitted""#), + "{line}" + ); assert!(line.ends_with("token=none"), "{line}"); } @@ -338,8 +356,10 @@ mod tests { whole.granted = whole.requested.clone(); whole.cut = Vec::new(); whole.rule = None; + whole.reason = None; let line = one_line(&whole); assert!(!line.contains("granted="), "{line}"); assert!(!line.contains("cut="), "{line}"); + assert!(!line.contains("reason="), "{line}"); } } diff --git a/crates/didbot-agentd/src/decisions.rs b/crates/didbot-agentd/src/decisions.rs index df1d207f..098b65ab 100644 --- a/crates/didbot-agentd/src/decisions.rs +++ b/crates/didbot-agentd/src/decisions.rs @@ -145,11 +145,15 @@ impl Record { impl From<&Record> for DecisionForAgent { fn from(record: &Record) -> Self { - let (verdict, cut, rule) = match &record.verdict { - Verdict::Allow => ("allow", Vec::new(), None), - Verdict::Narrow { cut, rule, .. } => ("narrow", cut.clone(), Some(rule.clone())), + // The rule and the reason stay two fields. The rule is an identifier + // an owner can look up in their own policy; the reason is a sentence + // for the agent to read. Joining them would leave an adapter to split + // a string on a colon to get either one back. + let (verdict, cut, rule, reason) = match &record.verdict { + Verdict::Allow => ("allow", Vec::new(), None, None), + Verdict::Narrow { cut, rule, .. } => ("narrow", cut.clone(), Some(rule.clone()), None), Verdict::Deny { reason, rule } => { - ("deny", Vec::new(), Some(format!("{rule}: {reason}"))) + ("deny", Vec::new(), Some(rule.clone()), Some(reason.clone())) } }; Self { @@ -160,6 +164,7 @@ impl From<&Record> for DecisionForAgent { granted: record.granted(), cut, rule, + reason, verdict: verdict.to_owned(), expires_at: record.expires_at.clone(), } @@ -429,6 +434,8 @@ mod tests { assert_eq!(shown.verdict, "narrow"); assert_eq!(shown.cut, vec!["repo:com.example.thing".to_owned()]); assert_eq!(shown.rule.as_deref(), Some("ceiling")); + // A narrowing has a rule and no reason: nothing was refused. + assert!(shown.reason.is_none()); assert_eq!(shown.token.as_deref(), Some("k7f3")); } @@ -564,9 +571,9 @@ mod tests { // No token, because there is no choice: the agent is being told what // happened, not asked. assert!(shown.token.is_none()); - assert_eq!( - shown.rule.as_deref(), - Some("app-allowlist: that client is not admitted") - ); + // Two fields, not one sentence: the rule is what an owner looks up, + // the reason is what the agent reads. + assert_eq!(shown.rule.as_deref(), Some("app-allowlist")); + assert_eq!(shown.reason.as_deref(), Some("that client is not admitted")); } } diff --git a/crates/didbot-agentd/src/protocol.rs b/crates/didbot-agentd/src/protocol.rs index 86c71de1..4aa28934 100644 --- a/crates/didbot-agentd/src/protocol.rs +++ b/crates/didbot-agentd/src/protocol.rs @@ -247,9 +247,17 @@ pub struct DecisionForAgent { /// What was taken out of the request, empty unless it was narrowed. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub cut: Vec, - /// The rule that narrowed or refused it. + /// The rule that narrowed or refused it, named on its own. #[serde(default, skip_serializing_if = "Option::is_none")] pub rule: Option, + /// Why it was refused, in the policy's own sentence. + /// + /// Separate from [`DecisionForAgent::rule`] rather than folded into it: + /// the rule is an identifier an owner can go and look up, the reason is + /// prose for the agent to read, and an adapter rendering them wants to + /// treat the two differently. Absent unless the verdict is `deny`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub reason: Option, /// `allow`, `narrow` or `deny`. pub verdict: String, /// When the request stops being answerable, RFC 3339. @@ -477,6 +485,7 @@ mod tests { granted: vec!["atproto".into()], cut: vec!["repo:com.example.thing".into()], rule: Some("ceiling".into()), + reason: None, verdict: "narrow".into(), expires_at: "2026-09-09T12:04:00Z".into(), }]); @@ -486,6 +495,45 @@ mod tests { ); } + #[test] + fn a_refusal_carries_the_rule_and_the_reason_as_two_fields() { + let answer = Answer::quiet().and_pending(vec![DecisionForAgent { + token: None, + client_origin: "http://127.0.0.1:40831".into(), + first_time: true, + requested: vec!["atproto".into()], + granted: Vec::new(), + cut: Vec::new(), + rule: Some("app-allowlist".into()), + reason: Some("that client is not admitted".into()), + verdict: "deny".into(), + expires_at: "2026-09-09T12:04:00Z".into(), + }]); + assert_eq!( + serde_json::to_string(&answer).unwrap(), + r#"{"version":2,"pending":[{"clientOrigin":"http://127.0.0.1:40831","firstTime":true,"requested":["atproto"],"granted":[],"rule":"app-allowlist","reason":"that client is not admitted","verdict":"deny","expiresAt":"2026-09-09T12:04:00Z"}]}"# + ); + } + + #[test] + fn and_a_verdict_that_refused_nothing_carries_no_reason() { + let answer = Answer::quiet().and_pending(vec![DecisionForAgent { + token: Some("k7f3".into()), + client_origin: "http://127.0.0.1:40831".into(), + first_time: false, + requested: vec!["atproto".into()], + granted: vec!["atproto".into()], + cut: Vec::new(), + rule: None, + reason: None, + verdict: "allow".into(), + expires_at: "2026-09-09T12:04:00Z".into(), + }]); + let line = serde_json::to_string(&answer).unwrap(); + assert!(!line.contains("reason"), "{line}"); + assert!(!line.contains("rule"), "{line}"); + } + #[test] fn an_empty_list_of_decisions_is_left_off_rather_than_sent() { assert_eq!( @@ -527,6 +575,7 @@ mod tests { granted: vec!["atproto".into()], cut: Vec::new(), rule: None, + reason: None, verdict: "allow".into(), expires_at: "2026-09-09T12:04:00Z".into(), }]); -- 2.51.2