radicle/canonical: Remove RawRules::matches

Testing revealed that the implementation violated RIP-0006, which
requires the most specific rule matching the target of `HEAD`
to use an `allow` of `delegates` and the document threshold.
`validate_head_rule` took the first match from `RawRules`, which orders
patterns lexicographically, so a glob could be validated in place of the
rule that actually governs the branch.
The issue here is that the correct order for matching patterns is only
guaranteed on `Rules`, but not on `RawRules`.

To prevent such errors in the future, remove `RawRules::matches`. This
requires some refactoring, and makes synthesis of the default branch
rule slightly less efficient in the worst case, but we are willing to
take that hit for a cleaner implementation.

Co-authored-by: Lorenz Leutgeb <lorenz.leutgeb@radicle.dev>

crates/radicle-cli/examples/git/git-push-canonical-symbolic-ref.md

@@ -1,17 +1,15 @@
-Bob overhears that the new name for the default branch is "main", not "master"
+Bob has a weird name for a particular release in mind, and also wants to test
-as it used to be. To make tooling that expect "main" work, without the hassle
+out symbolic references.
-of having to push to such branch manually, he opts to create a symbolic
-reference.
 
 ``` ~bob
 $ cd heartwood
-$ rad id update --title "Add canonical symbolic ref" --payload xyz.radicle.crefs symbolic '{ "refs/heads/main": "refs/heads/master" }'
+$ rad id update --title "Add canonical symbolic ref" --payload xyz.radicle.crefs symbolic '{ "refs/heads/foobar": "refs/heads/releases/foobaz" }'
 ✓ Identity revision [..] created
 ╭────────────────────────────────────────────────────────────────────────╮
 │ Title    Add canonical symbolic ref                                    │
-│ Revision 62e2cb60c6df9ad9908b6697b5d126760a855484                      │
+│ Revision b7597e3eeaba64467db4417602518169b76b43d5                      │
 │ Parent   37a1aad231100cd206c49aed79e405ea2da9204b                      │
-│ Blob     b20de7b184673eb0d9227be17640c923d8ef3f3e                      │
+│ Blob     49a21614cf358b131fd6b590d0d01396f98906ce                      │
 │ Author   did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk      │
 │ State    active                                                        │
 │ Quorum   no                                                            │
@@ -41,7 +39,7 @@ $ rad id update --title "Add canonical symbolic ref" --payload xyz.radicle.crefs
          }
 +      },
 +      "symbolic": {
-+        "refs/heads/main": "refs/heads/master"
++        "refs/heads/foobar": "refs/heads/releases/foobaz"
        }
      },
      "xyz.radicle.project": {
@@ -58,14 +56,22 @@ $ rad id update --title "Add canonical symbolic ref" --payload xyz.radicle.crefs
  }
 ```
 
-Alice is happy with the new revision and accepts it.
+Bob proceeds to push some commit to the branch that is targeted by the
+symbolic reference. 
+
+``` ~bob
+$ git push -q rad master:releases/foobaz
+```
+
+Alice decides to play along with Bob's strange ideas and accepts the new
+revision.
 
 ``` ~alice
 $ rad sync -f
 Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from the network, found 1 potential seed(s).
 ✓ Target met: 1 seed(s)
 🌱 Fetched from z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk
-$ rad id accept 62e2cb60c6df9ad9908b6697b5d126760a855484 -q
+$ rad id accept b7597e3eeaba64467db4417602518169b76b43d5 -q
 ```
 
 As usual, alice works on "master".
@@ -91,9 +97,10 @@ She inspects the remote and sees that indeed a new branch named "main" now exist
 ``` ~alice 
 $ git ls-remote rad
 4dc510ddea5fd66499d1d2e996b8a97c8d57be54	HEAD
-4dc510ddea5fd66499d1d2e996b8a97c8d57be54	refs/heads/main
+f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354	refs/heads/foobar
 4dc510ddea5fd66499d1d2e996b8a97c8d57be54	refs/heads/master
 afec366785ed3651cdc66975c0fec41866c9ce62	refs/heads/releases/2
+f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354	refs/heads/releases/foobaz
 f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354	refs/tags/qa/v2.1
 ac51a0746a5e8311829bc481202909a1e3acc0c2	refs/tags/v1.0-hotfix
 89f935f27a16f8ed97915ade4accab8fe48057aa	refs/tags/v2.0
@@ -104,10 +111,11 @@ Of course, she can also fetch it to her working copy as usual.
 ``` ~alice (stderr)
 $ git fetch rad
 From rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji
- * [new branch]      main       -> rad/main
+ * [new branch]      foobar          -> rad/foobar
- * [new branch]      releases/2 -> rad/releases/2
+ * [new branch]      releases/2      -> rad/releases/2
- * [new tag]         qa/v2.1    -> rad/tags/qa/v2.1
+ * [new branch]      releases/foobaz -> rad/releases/foobaz
- * [new tag]         qa/v2.1    -> qa/v2.1
+ * [new tag]         qa/v2.1         -> rad/tags/qa/v2.1
+ * [new tag]         qa/v2.1         -> qa/v2.1
 ```
 
 Bob fetches Alice's changes.
@@ -124,8 +132,8 @@ And, sure enough, there is the new branch just as he wanted it.
 ``` ~bob (stderr)
 $ git fetch rad
 From rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji
- * [new branch]      main       -> rad/main
+ * [new branch]      foobar     -> rad/foobar
    f2de534..4dc510d  master     -> rad/master
 ```
 
-Note that neither Alice nor Bob pushed directly to "main".
+Note that neither Alice nor Bob pushed directly to "foobar".

crates/radicle/src/git/canonical/rules.rs

@@ -32,9 +32,9 @@ use super::protect::Unprotected;
 
 const ASTERISK: char = '*';
 
-/// Private trait to ensure that not any `Rule` can be deserialized.
+/// Private trait to ensure that not any [`Rule`] can be deserialized.
-/// Implementations are provided for `Allowed` and `usize` so that `RawRule`s
+/// Implementations are provided for [`Allowed`] and [`usize`] so that [`RawRule`]s
-/// can be deserialized, while `ValidRule`s cannot – preventing deserialization
+/// can be deserialized, while [`ValidRule`]s cannot – preventing deserialization
 /// bugs for that type.
 trait Sealed {}
 impl Sealed for Allowed {}
@@ -246,7 +246,13 @@ impl RawRule {
     }
 }
 
-/// A set of `RawRule`s that can be serialized and deserialized.
+/// A set of [`RawRule`]s that can be serialized and deserialized.
+///
+/// Note that matching on the [`RawPattern`]s is deliberately not implemented,
+/// as the [`RawRule`]s are not validated and thus should not be used to
+/// calculate canonical references.
+/// If you want to match on a set of rules, obtain [`Rules`] via
+/// [`Rules::from_raw`], and use [`Rules::matches`] on the result.
 #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)]
 pub struct RawRules {
     /// The reference pattern that this rule applies to.
@@ -285,18 +291,6 @@ impl RawRules {
             .iter()
             .any(|(pattern, _)| pattern.as_str() == refname)
     }
-
-    /// Check if the `refname` matches any existing rules, including glob
-    /// matches.
-    pub fn matches<'a, 'b>(
-        &self,
-        refname: &Qualified<'b>,
-    ) -> impl Iterator<Item = (&RawPattern, &RawRule)> + use<'a, '_, 'b> {
-        let refname = refname.clone();
-        self.rules
-            .iter()
-            .filter(move |(pattern, _)| matches(pattern, &refname))
-    }
 }
 
 impl Extend<(RawPattern, RawRule)> for RawRules {

crates/radicle/src/git/canonical/rules/test.rs

@@ -563,3 +563,45 @@ fn matches_exactly_curly_braces() {
     assert!(!matches(&exact, &git::fmt::qualified!("refs/heads/foo")));
     assert!(!matches(&exact, &git::fmt::qualified!("refs/heads/bar")));
 }
+
+#[test]
+fn most_specific_match_ignores_lexicographic_order() {
+    use helper::*;
+
+    let glob = raw_pattern(qualified_pattern!("refs/heads/*"));
+    let nested = raw_pattern(qualified_pattern!("refs/heads/feature/*"));
+    let exact = raw_pattern(qualified_pattern!("refs/heads/main"));
+    let rules = RawRules::from_iter([
+        (glob.clone(), Rule::new(Allowed::Delegates, 1)),
+        (nested.clone(), Rule::new(Allowed::Delegates, 1)),
+        (exact.clone(), Rule::new(Allowed::Delegates, 1)),
+    ]);
+
+    let resolve = &mut || {
+        doc::Delegates::from(
+            Did::decode("did:key:z6MkpQTLwr8QyADGmBGAMsGttvWzP4PojUMs4hREZW5T5E3K").unwrap(),
+        )
+    };
+
+    let rules = Rules::from_raw(rules, resolve).unwrap();
+
+    let main = git::fmt::qualified!("refs/heads/main");
+    assert_eq!(
+        rules.matches(&main).next().map(|(pattern, _)| pattern),
+        Some(&exact)
+    );
+
+    let feature = git::fmt::qualified!("refs/heads/feature/x");
+    assert_eq!(
+        rules.matches(&feature).next().map(|(pattern, _)| pattern),
+        Some(&nested),
+        "more components beat the glob"
+    );
+
+    assert!(
+        rules
+            .matches(&git::fmt::qualified!("refs/tags/v1.0"))
+            .next()
+            .is_none()
+    );
+}

crates/radicle/src/identity/crefs.rs

@@ -132,6 +132,10 @@ impl CanonicalRefs {
     pub fn symbolic(&self) -> &SymbolicRefs {
         &self.symbolic
     }
+
+    pub fn into_raw(self) -> (RawRules, SymbolicRefs) {
+        (self.rules.into(), self.symbolic)
+    }
 }
 
 impl Extend<(rules::RawPattern, rules::RawRule)> for RawCanonicalRefs {

crates/radicle/src/identity/doc.rs

@@ -19,7 +19,7 @@ use crate::cob::identity;
 use crate::crypto;
 use crate::crypto::Signature;
 use crate::git;
-use crate::git::canonical::rules;
+use crate::git::canonical::rules::{self, ResolvedDelegates};
 use crate::git::canonical::symbolic;
 use crate::git::fmt::Qualified;
 use crate::git::fmt::RefString;
@@ -808,17 +808,20 @@ impl Doc {
     ///
     /// [`RawCanonicalRefs`]: super::crefs::RawCanonicalRefs
     pub fn canonical_refs(&self) -> Result<CanonicalRefs, CanonicalRefsError> {
-        let mut raw_crefs = self
+        let resolve = &mut || self.delegates.clone();
+        let crefs = self
             .raw_canonical_refs()
             .transpose()
             .map_err(CanonicalRefsError::Payload)?
+            .map(|crefs| crefs.try_into_canonical_refs(resolve))
+            .transpose()
+            .map_err(CanonicalRefsError::CanonicalRefs)?
             .unwrap_or_default();
-        let resolve = &mut || self.delegates.clone();
 
         // Determine where `HEAD` comes from. The `resolve_head()` result
         // borrows `raw_crefs`, so clone to allow mutation in the synthesis
         // path.
-        let head: Option<Qualified<'static>> = raw_crefs.symbolic().resolve_head().cloned();
+        let head: Option<Qualified<'static>> = crefs.symbolic().resolve_head().cloned();
 
         match (head, self.project()) {
             (Some(ref default_branch), Some(Ok(project))) => {
@@ -831,13 +834,14 @@ impl Doc {
                         },
                     ));
                 }
-                self.validate_head_rule(&raw_crefs, default_branch)?;
+                self.validate_head_rule(&crefs, default_branch)?;
             }
             (Some(ref default_branch), None) => {
-                self.validate_head_rule(&raw_crefs, default_branch)?;
+                self.validate_head_rule(&crefs, default_branch)?;
             }
             (None, Some(Ok(project))) => {
-                self.synthesize_head(&mut raw_crefs, &project)?;
+                let raw_crefs = self.synthesize_head(crefs, &project)?;
+                return Ok(raw_crefs.try_into_canonical_refs(resolve)?);
             }
             (None, None) => {
                 return Err(CanonicalRefsError::SynthesisPayloadMissing);
@@ -847,7 +851,7 @@ impl Doc {
             }
         }
 
-        Ok(raw_crefs.try_into_canonical_refs(resolve)?)
+        Ok(crefs)
     }
 
     /// Validate that the rule matching `HEAD`'s target branch uses
@@ -857,23 +861,32 @@ impl Doc {
     /// later by [`RawCanonicalRefs::try_into_canonical_refs`] validation.
     fn validate_head_rule(
         &self,
-        raw_crefs: &RawCanonicalRefs,
+        raw_crefs: &CanonicalRefs,
-        default_branch: &Qualified,
+        default_branch: &Qualified<'static>,
     ) -> Result<(), CanonicalRefsError> {
-        let Some((pattern, rule)) = raw_crefs.raw_rules().matches(default_branch).next() else {
+        let Some((pattern, rule)) = raw_crefs.rules().matches(default_branch).next() else {
             return Ok(());
         };
 
-        let allowed = rule.allowed();
+        let _allowed = rule.allowed();
-        if *allowed != rules::Allowed::Delegates {
+        match rule.allowed() {
-            return Err(CanonicalRefsError::DefaultBranchRuleError(
+            ResolvedDelegates::Delegates(_) => {
-                DefaultBranchRuleError::Allowed {
+                // This is the expected case, so we continue to validate the threshold.
-                    pattern: pattern.to_string(),
+            }
-                    actual: allowed.to_string(),
+            ResolvedDelegates::Set(actual) => {
-                },
+                return Err(CanonicalRefsError::DefaultBranchRuleError(
-            ));
+                    DefaultBranchRuleError::Allowed {
+                        pattern: pattern.to_string(),
+                        actual: actual
+                            .iter()
+                            .map(|s| s.to_string())
+                            .collect::<Vec<_>>()
+                            .join(", "),
+                    },
+                ));
+            }
         }
-        let actual = *rule.threshold();
+        let actual = (*rule.threshold()).into();
         let expected = self.threshold();
         if actual != expected {
             return Err(CanonicalRefsError::DefaultBranchRuleError(
@@ -891,30 +904,28 @@ impl Doc {
     /// from the project payload.
     fn synthesize_head(
         &self,
-        raw_crefs: &mut RawCanonicalRefs,
+        crefs: CanonicalRefs,
         project: &Project,
-    ) -> Result<(), CanonicalRefsError> {
+    ) -> Result<RawCanonicalRefs, CanonicalRefsError> {
         let default_branch = project.default_branch_qualified();
 
-        if raw_crefs
+        let add_rule = crefs.rules().matches(&default_branch).next().is_none();
-            .raw_rules()
+
-            .matches(&default_branch)
+        let (mut raw_rules, mut symbolic) = crefs.into_raw();
-            .next()
+
-            .is_none()
+        if add_rule {
-        {
+            raw_rules.insert(
-            raw_crefs.raw_rules_mut().insert(
                 git::fmt::refspec::QualifiedPattern::from(default_branch.to_owned()),
                 rules::Rule::new(rules::Allowed::Delegates, self.threshold()),
             );
         }
 
         #[allow(deprecated)]
-        raw_crefs
+        symbolic
-            .symbolic_mut()
             .combine(symbolic::SymbolicRefs::head(project.default_branch()))
             .map_err(|source| CanonicalRefsError::SynthesisCycle { source })?;
 
-        Ok(())
+        Ok(RawCanonicalRefs::new(raw_rules, symbolic))
     }
 
     /// Return the associated [`Visibility`] of this document.

crates/radicle/src/identity/doc/update.rs

@@ -208,21 +208,38 @@ pub fn verify(raw: RawDoc) -> Result<Doc, error::DocVerification> {
         }
     };
 
+    let resolve = &mut || proposal.delegates.clone();
+
+    let crefs = match proposal.raw_canonical_refs() {
+        None => None,
+        Some(Ok(crefs)) => match crefs.try_into_canonical_refs(resolve) {
+            Ok(crefs) => Some(crefs),
+            Err(err) => {
+                return Err(error::DocVerification::PayloadError {
+                    id: PayloadId::canonical_refs().clone(),
+                    err: err.to_string(),
+                });
+            }
+        },
+        Some(Err(PayloadError::Json(e))) => {
+            return Err(error::DocVerification::PayloadError {
+                id: PayloadId::canonical_refs().clone(),
+                err: e.to_string(),
+            });
+        }
+    };
+
     // If we have both payloads `xyz.radicle.{project,crefs}` ensure that,
     // in the `crefs` payload there is no …
     //  1. … rule that matches the default branch from the  `project` payload.
     //     (This rule must be synthesized!)
     //  2. … symbolic reference with the name `HEAD`.
     //     (This reference must be synthesized!)
-    match raw
+    match crefs.zip(project) {
-        .raw_canonical_refs()
+        Some((crefs, project)) => {
-        .transpose()
-        .map(|rcrefs| rcrefs.zip(project))
-    {
-        Ok(Some((crefs, project))) => {
             let default = project.default_branch_qualified().to_owned();
             let matches = crefs
-                .raw_rules()
+                .rules()
                 .matches(&default)
                 .map(|(pattern, _)| pattern.to_string())
                 .collect::<Vec<_>>();
@@ -240,13 +257,6 @@ pub fn verify(raw: RawDoc) -> Result<Doc, error::DocVerification> {
         _ => { /* we validate below */ }
     }
 
-    // Verify that the canonical references payload is valid
-    if let Err(e) = proposal.canonical_refs() {
-        return Err(error::DocVerification::PayloadError {
-            id: PayloadId::canonical_refs().clone(),
-            err: e.to_string(),
-        });
-    }
     Ok(proposal)
 }