From eb2a7c4e5419ff0a3bb7c325bdf6fb8044c71946 Mon Sep 17 00:00:00 2001 From: Clement Delafargue Date: Thu, 26 Oct 2023 14:52:13 +0200 Subject: [PATCH] errors: display more info about failed checks and policies --- biscuit-auth/src/error.rs | 66 ++++++++++++++++++++++++---- biscuit-auth/src/token/authorizer.rs | 54 +++++++++++++++++++++++ biscuit-capi/tests/capi.rs | 2 +- 3 files changed, 112 insertions(+), 10 deletions(-) diff --git a/biscuit-auth/src/error.rs b/biscuit-auth/src/error.rs index 5690e3eb..4e61adbe 100644 --- a/biscuit-auth/src/error.rs +++ b/biscuit-auth/src/error.rs @@ -1,7 +1,10 @@ //! error types //! -use std::convert::{From, Infallible}; +use std::{ + convert::{From, Infallible}, + fmt::Display, +}; use thiserror::Error; /// the global error type for Biscuit @@ -16,7 +19,7 @@ pub enum Token { AppendOnSealed, #[error("tried to seal an already sealed token")] AlreadySealed, - #[error("authorization failed")] + #[error("authorization failed: {0}")] FailedLogic(Logic), #[error("error generating Datalog: {0}")] Language(biscuit_parser::error::LanguageError), @@ -168,9 +171,9 @@ pub enum Signature { #[derive(Error, Clone, Debug, PartialEq, Eq)] #[cfg_attr(feature = "serde-error", derive(serde::Serialize, serde::Deserialize))] pub enum Logic { - #[error("a rule provided by a block is generating facts with the authority or ambient tag, or has head variables not used in its body")] + #[error("a rule provided by a block is producing a fact with unbound variables")] InvalidBlockRule(u32, String), - #[error("authorization failed")] + #[error("{policy}, and the following checks failed: {}", display_failed_checks(.checks))] Unauthorized { /// the policy that matched policy: MatchedPolicy, @@ -179,7 +182,7 @@ pub enum Logic { }, #[error("the authorizer already contains a token")] AuthorizerNotEmpty, - #[error("no matching policy was found")] + #[error("no matching policy was found, and the following checks failed: {}", display_failed_checks(.checks))] NoMatchingPolicy { /// list of checks that failed validation checks: Vec, @@ -189,9 +192,9 @@ pub enum Logic { #[derive(Error, Clone, Debug, PartialEq, Eq)] #[cfg_attr(feature = "serde-error", derive(serde::Serialize, serde::Deserialize))] pub enum MatchedPolicy { - #[error("an allow policy matched")] + #[error("an allow policy matched (policy index: {0})")] Allow(usize), - #[error("a deny policy matched")] + #[error("a deny policy matched (policy index: {0})")] Deny(usize), } @@ -199,12 +202,19 @@ pub enum MatchedPolicy { #[derive(Error, Clone, Debug, PartialEq, Eq)] #[cfg_attr(feature = "serde-error", derive(serde::Serialize, serde::Deserialize))] pub enum FailedCheck { - #[error("a check failed in a block")] + #[error("{0}")] Block(FailedBlockCheck), - #[error("a check provided by the authorizer failed")] + #[error("{0}")] Authorizer(FailedAuthorizerCheck), } +fn display_failed_checks(c: &[FailedCheck]) -> String { + c.iter() + .map(|c| c.to_string()) + .collect::>() + .join(", ") +} + #[derive(Clone, Debug, PartialEq, Eq)] #[cfg_attr(feature = "serde-error", derive(serde::Serialize, serde::Deserialize))] pub struct FailedBlockCheck { @@ -214,6 +224,16 @@ pub struct FailedBlockCheck { pub rule: String, } +impl Display for FailedBlockCheck { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "Check n°{} in block n°{}: {}", + self.check_id, self.block_id, self.rule + ) + } +} + #[derive(Clone, Debug, PartialEq, Eq)] #[cfg_attr(feature = "serde-error", derive(serde::Serialize, serde::Deserialize))] pub struct FailedAuthorizerCheck { @@ -222,6 +242,12 @@ pub struct FailedAuthorizerCheck { pub rule: String, } +impl Display for FailedAuthorizerCheck { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "Check n°{} in authorizer: {}", self.check_id, self.rule) + } +} + /// Datalog execution errors #[derive(Error, Clone, Debug, PartialEq, Eq)] #[cfg_attr(feature = "serde-error", derive(serde::Serialize, serde::Deserialize))] @@ -283,5 +309,27 @@ mod tests { format!("{}", Token::Base64(Base64Error::InvalidLength)), "Cannot decode base64 token: Encoded text cannot have a 6-bit remainder." ); + + assert_eq!( + format!( + "{}", + Token::FailedLogic(Logic::Unauthorized { + policy: MatchedPolicy::Allow(0), + checks: vec![ + FailedCheck::Authorizer(FailedAuthorizerCheck { + check_id: 0, + rule: "check if false".to_string() + }), + FailedCheck::Block(FailedBlockCheck { + block_id: 0, + check_id: 0, + rule: "check if false".to_string() + }) + ] + }) + ) + .to_string(), + "authorization failed: an allow policy matched (policy index: 0), and the following checks failed: Check n°0 in authorizer: check if false, Check n°0 in block n°0: check if false" + ); } } diff --git a/biscuit-auth/src/token/authorizer.rs b/biscuit-auth/src/token/authorizer.rs index dcba5ea4..f43f1d01 100644 --- a/biscuit-auth/src/token/authorizer.rs +++ b/biscuit-auth/src/token/authorizer.rs @@ -1378,6 +1378,8 @@ impl AuthorizerExt for Authorizer { mod tests { use std::time::Duration; + use token::{public_keys::PublicKeys, DATALOG_3_1}; + use crate::{ builder::{Algorithm, BiscuitBuilder, BlockBuilder}, KeyPair, @@ -1815,4 +1817,56 @@ allow if true; let authorizer = Authorizer::new(); assert_eq!("", authorizer.to_string()) } + + #[test] + fn rule_validate_variables() { + let mut authorizer = Authorizer::new(); + let mut syms = SymbolTable::new(); + let rule_name = syms.insert("test"); + let pred_name = syms.insert("pred"); + let rule = datalog::rule( + rule_name, + &[datalog::var(&mut syms, "unbound")], + &[datalog::pred(pred_name, &[datalog::var(&mut syms, "any")])], + ); + let mut block = Block { + symbols: syms.clone(), + facts: vec![], + rules: vec![rule], + checks: vec![], + context: None, + version: DATALOG_3_1, + external_key: None, + public_keys: PublicKeys::new(), + scopes: vec![], + }; + + assert_eq!( + authorizer + .load_and_translate_block(&mut block, 0, &syms) + .unwrap_err(), + error::Token::FailedLogic(error::Logic::InvalidBlockRule( + 0, + "test($unbound) <- pred($any)".to_string() + )) + ); + + // broken rules directly added to the authorizer currently don’t trigger any error, but silently fail to generate facts when they match + authorizer + .add_rule(builder::rule( + "test", + &[var("unbound")], + &[builder::pred("pred", &[builder::var("any")])], + )) + .unwrap(); + let res: Vec<(String,)> = authorizer + .query(builder::rule( + "output", + &[builder::string("x")], + &[builder::pred("test", &[builder::var("any")])], + )) + .unwrap(); + + assert_eq!(res, vec![]); + } } diff --git a/biscuit-capi/tests/capi.rs b/biscuit-capi/tests/capi.rs index 5bf9213a..a168f2e6 100644 --- a/biscuit-capi/tests/capi.rs +++ b/biscuit-capi/tests/capi.rs @@ -114,7 +114,7 @@ biscuit append error? (null) authorizer creation error? (null) authorizer add check error? (null) authorizer add policy error? (null) -authorizer error(code = 21): authorization failed +authorizer error(code = 21): authorization failed: an allow policy matched (policy index: 0), and the following checks failed: Check n°0 in authorizer: check if right("efgh"), Check n°0 in block n°1: check if operation("read") failed checks (2): Authorizer check 0: check if right("efgh") Block 1, check 0: check if operation("read")