diff --git a/crates/radicle-node/src/worker/fetch/error.rs b/crates/radicle-node/src/worker/fetch/error.rs index a9111a6a..6c9d753f 100644 --- a/crates/radicle-node/src/worker/fetch/error.rs +++ b/crates/radicle-node/src/worker/fetch/error.rs @@ -71,5 +71,5 @@ pub enum Canonical { #[error(transparent)] Identity(#[from] radicle::storage::RepositoryError), #[error(transparent)] - Payload(#[from] radicle::identity::PayloadError), + CanonicalRefs(#[from] radicle::identity::doc::CanonicalRefsError), } diff --git a/crates/radicle-remote-helper/src/push.rs b/crates/radicle-remote-helper/src/push.rs index 8d21062d..0c3f0ec0 100644 --- a/crates/radicle-remote-helper/src/push.rs +++ b/crates/radicle-remote-helper/src/push.rs @@ -6,7 +6,7 @@ use std::str::FromStr; use std::{assert_eq, io}; use radicle::identity::crefs::GetCanonicalRefs as _; -use radicle::identity::doc::DefaultBranchRuleError; +use radicle::identity::doc::CanonicalRefsError; use radicle::node::device::Device; use thiserror::Error; @@ -118,7 +118,7 @@ pub enum Error { #[error(transparent)] Quorum(#[from] radicle::git::canonical::QuorumError), #[error(transparent)] - DefaultBranchRule(#[from] radicle::identity::doc::DefaultBranchRuleError), + CanonicalRefs(#[from] radicle::identity::doc::CanonicalRefsError), } /// Push command. @@ -271,7 +271,7 @@ pub fn run( let identity = stored.identity()?; let crefs = identity.canonical_refs_or_default(|| { let rule = identity.doc().default_branch_rule()?; - Ok::<_, DefaultBranchRuleError>(CanonicalRefs::from_iter([rule])) + Ok::<_, CanonicalRefsError>(CanonicalRefs::from_iter([rule])) })?; let rules = crefs.rules(); let me = Did::from(nid); diff --git a/crates/radicle/src/identity/crefs.rs b/crates/radicle/src/identity/crefs.rs index 112d5ccf..939d5b16 100644 --- a/crates/radicle/src/identity/crefs.rs +++ b/crates/radicle/src/identity/crefs.rs @@ -69,7 +69,7 @@ impl RawCanonicalRefs { R: Fn() -> Delegates, { let rules = Rules::from_raw(self.rules, resolve)?; - Ok(CanonicalRefs { rules }) + Ok(CanonicalRefs::new(rules)) } } diff --git a/crates/radicle/src/identity/doc.rs b/crates/radicle/src/identity/doc.rs index 169876ae..db27c47d 100644 --- a/crates/radicle/src/identity/doc.rs +++ b/crates/radicle/src/identity/doc.rs @@ -241,8 +241,6 @@ impl PayloadId { #[derive(Debug, Error)] pub enum PayloadError { - #[error(transparent)] - CanonicalRefs(#[from] rules::ValidationError), #[error("json: {0}")] Json(#[from] serde_json::Error), #[error("payload '{0}' not found in identity document")] @@ -914,14 +912,32 @@ impl Doc { } } +#[derive(Debug, Error)] +pub enum CanonicalRefsError { + #[error(transparent)] + Json(#[from] serde_json::Error), + #[error(transparent)] + CanonicalRefs(#[from] rules::ValidationError), + #[error(transparent)] + DefaultBranch(#[from] DefaultBranchRuleError), +} + impl crefs::GetCanonicalRefs for Doc { - type Error = PayloadError; + type Error = CanonicalRefsError; fn canonical_refs(&self) -> Result, Self::Error> { self.raw_canonical_refs().and_then(|raw| { raw.map(|raw| { raw.try_into_canonical_refs(&mut || self.delegates.clone()) - .map_err(PayloadError::from) + .map_err(CanonicalRefsError::from) + .and_then(|mut crefs| { + self.default_branch_rule() + .map_err(CanonicalRefsError::from) + .map(|rule| { + crefs.extend([rule]); + crefs + }) + }) }) .transpose() }) @@ -930,7 +946,27 @@ impl crefs::GetCanonicalRefs for Doc { fn raw_canonical_refs(&self) -> Result, Self::Error> { let value = self.payload.get(&PayloadId::canonical_refs()); let crefs = value - .map(|value| serde_json::from_value((**value).clone()).map_err(PayloadError::from)) + .map(|value| { + serde_json::from_value((**value).clone()).map_err(CanonicalRefsError::from) + }) + .transpose()?; + Ok(crefs) + } +} + +impl crefs::GetCanonicalRefs for RawDoc { + type Error = CanonicalRefsError; + + fn canonical_refs(&self) -> Result, Self::Error> { + Ok(None) + } + + fn raw_canonical_refs(&self) -> Result, Self::Error> { + let value = self.payload.get(&PayloadId::canonical_refs()); + let crefs = value + .map(|value| { + serde_json::from_value((**value).clone()).map_err(CanonicalRefsError::from) + }) .transpose()?; Ok(crefs) } diff --git a/crates/radicle/src/identity/doc/update.rs b/crates/radicle/src/identity/doc/update.rs index 14662319..9a55f681 100644 --- a/crates/radicle/src/identity/doc/update.rs +++ b/crates/radicle/src/identity/doc/update.rs @@ -5,12 +5,13 @@ use std::{collections::BTreeSet, str::FromStr}; use serde_json as json; use crate::{ + git, identity::crefs::GetCanonicalRefs as _, prelude::Did, storage::{refs, ReadRepository, RepositoryError}, }; -use super::{Doc, PayloadId, RawDoc, Visibility}; +use super::{Doc, PayloadError, PayloadId, RawDoc, Visibility}; /// [`EditVisibility`] allows the visibility of a [`RawDoc`] to be edited using /// the [`visibility`] function. @@ -186,18 +187,44 @@ pub fn payload( /// [`Project`]: crate::identity::Project /// [`CanonicalRefs`]: crate::identity::CanonicalRefs pub fn verify(raw: RawDoc) -> Result { - let proposal = raw.verified()?; - // Verify that the payloads can still be parsed into the correct types. - if let Err(super::PayloadError::Json(e)) = proposal.project() { - return Err(error::DocVerification::PayloadJson { - id: PayloadId::project(), - err: e, - }); + let proposal = raw.clone().verified()?; + // Verify that the project payload is valid + // TODO(finto): perhaps this should be handled by JSON Schemas instead + let project = match proposal.project() { + Ok(project) => Some(project), + Err(PayloadError::NotFound(_)) => None, + Err(PayloadError::Json(e)) => { + return Err(error::DocVerification::PayloadError { + id: PayloadId::project(), + err: e.to_string(), + }) + } + }; + // Ensure that if we have canonical reference rules and a project, that no + // rule exists for the default branch. This rule must be synthesized when + // constructing the canonical reference rules. + match raw + .raw_canonical_refs() + .map(|rcrefs| rcrefs.and_then(|c| project.map(|p| (c, p)))) + { + Ok(Some((crefs, project))) => { + let default = git::Qualified::from(git::lit::refs_heads(project.default_branch())); + let matches = crefs + .raw_rules() + .matches(&default) + .map(|(pattern, _)| pattern.to_string()) + .collect::>(); + if !matches.is_empty() { + return Err(error::DocVerification::DisallowDefault { matches, default }); + } + } + _ => { /* we validate below */ } } - if let Err(super::PayloadError::Json(e)) = proposal.canonical_refs() { - return Err(error::DocVerification::PayloadJson { + // Verify that the canonical references payload is valid + if let Err(e) = proposal.canonical_refs() { + return Err(error::DocVerification::PayloadError { id: PayloadId::canonical_refs(), - err: e, + err: e.to_string(), }); } Ok(proposal) @@ -233,3 +260,101 @@ where Ok((dids.len() - missing.len() < threshold).then_some(missing)) } + +#[allow(clippy::unwrap_used)] +#[cfg(test)] +mod test { + use serde_json::json; + + use crate::{ + git, + identity::{ + crefs::GetCanonicalRefs, + doc::{update::error, PayloadId}, + }, + prelude::RawDoc, + test::arbitrary, + }; + + #[test] + fn test_can_update_crefs() { + let raw = arbitrary::gen::(1); + let raw = super::payload( + raw, + vec![( + PayloadId::canonical_refs(), + "rules".to_string(), + json!({ + "refs/tags/*": { + "threshold": 1, + "allow": "delegates" + } + }), + )], + ) + .unwrap(); + let verified = super::verify(raw); + assert!(verified.is_ok(), "Unexpected error {:?}", verified); + } + + #[test] + fn test_cannot_include_default_branch_rule() { + let raw = arbitrary::gen::(1); + let branch = git::Qualified::from(git::lit::refs_heads( + raw.project().unwrap().default_branch(), + )); + let raw = super::payload( + raw, + vec![( + PayloadId::canonical_refs(), + "rules".to_string(), + json!({ + "refs/tags/*": { + "threshold": 1, + "allow": "delegates" + }, + branch.as_str(): { + "threshold": 1, + "allow": "delegates", + } + }), + )], + ) + .unwrap(); + assert!( + matches!( + super::verify(raw), + Err(error::DocVerification::DisallowDefault { .. }) + ), + "Verification should be rejected for including default branch rule" + ) + } + + #[test] + fn test_default_branch_rule_exists_after_verification() { + let raw = arbitrary::gen::(1); + let branch = git::Qualified::from(git::lit::refs_heads( + raw.project().unwrap().default_branch(), + )); + let raw = super::payload( + raw, + vec![( + PayloadId::canonical_refs(), + "rules".to_string(), + json!({ + "refs/tags/*": { + "threshold": 1, + "allow": "delegates" + } + }), + )], + ) + .unwrap(); + let verified = super::verify(raw).unwrap(); + let crefs = verified.canonical_refs().unwrap().unwrap(); + assert!( + crefs.rules().matches(&branch).next().is_some(), + "Default branch rule is missing!" + ); + } +} diff --git a/crates/radicle/src/identity/doc/update/error.rs b/crates/radicle/src/identity/doc/update/error.rs index 02ebd910..a9e664f4 100644 --- a/crates/radicle/src/identity/doc/update/error.rs +++ b/crates/radicle/src/identity/doc/update/error.rs @@ -1,6 +1,6 @@ -use serde_json as json; use thiserror::Error; +use crate::git; use crate::git::RefString; use crate::identity::{doc::PayloadId, Did, DocError}; @@ -25,9 +25,14 @@ pub enum PayloadError { #[derive(Debug, Error)] pub enum DocVerification { #[error("failed to verify `{id}`, {err}")] - PayloadJson { id: PayloadId, err: json::Error }, + PayloadError { id: PayloadId, err: String }, #[error(transparent)] Doc(#[from] DocError), + #[error("incompatible payloads: The rule(s) xyz.radicle.crefs.rules.{matches:?} matches the value of xyz.radicle.project.defaultBranch ('{default}'). Possible resolutions: Change the name of the default branch or remove the rule(s).")] + DisallowDefault { + matches: Vec, + default: git::Qualified<'static>, + }, } #[derive(Clone, Debug)] diff --git a/crates/radicle/src/storage.rs b/crates/radicle/src/storage.rs index bff28f13..dab7f1ed 100644 --- a/crates/radicle/src/storage.rs +++ b/crates/radicle/src/storage.rs @@ -124,6 +124,8 @@ pub enum RepositoryError { MissingBranchRule, #[error("could not get the default branch rule: {0}")] DefaultBranchRule(#[from] doc::DefaultBranchRuleError), + #[error("failed to get canonical reference rules: {0}")] + CanonicalRefs(#[from] doc::CanonicalRefsError), } impl RepositoryError {