Merge pull request #191 from biscuit-auth/better-display-for-logic-error

Better display for `errors::Logic`

Author: divarvel

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<FailedCheck>,
@@ -189,22 +192,29 @@ 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),
 }
 
 /// check errors
 #[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::<Vec<_>>()
+        .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"
+        );
     }
 }

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![]);
+    }
 }

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